Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35001

convert_to_array anti-cycle algorithm messes with valid data

    Details

    • Testing Instructions:
      Hide

      Can not be tested directly but we can test that context is passed to filepicker correctly:

      1. As admin/teacher inside any course open filepicker (from any resource filemanager or tinyMCE textarea)
      2. Make sure that when you go to 'Server files' you are by default in the root folder of the current course
      3. If this course has legacy files, you can see them in filepicker
      Show
      Can not be tested directly but we can test that context is passed to filepicker correctly: As admin/teacher inside any course open filepicker (from any resource filemanager or tinyMCE textarea) Make sure that when you go to 'Server files' you are by default in the root folder of the current course If this course has legacy files, you can see them in filepicker
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-35001-master

      Description

      The function convert_to_array() loses valid data.

      For example, it doesn't accept:

      array('first' => array('second'), 'third' => array('second'));

      By passing this array to convert_to_array, it will only return: array('first' => array('second'));

      This is caused by the anti-cycle checking. While similar code may work in other languages, in PHP, arrays with the same value can't be differentiated, so you can't check if an array is the same instance and another.

      function convert_to_array($var) {
          $result = array();
          $references = array();
          // loop over elements/properties
          foreach ($var as $key => $value) {
              // recursively convert objects
              if (is_object($value) || is_array($value)) {
                  // but prevent cycles
                  if (!in_array($value, $references)) {          // PROBLEM HERE
                      $result[$key] = convert_to_array($value);
                      $references[] = $value;
                  }
              } else {
                  // simple values are untouched
                  $result[$key] = $value;
              }
          }
          return $result;
      }

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for reporting this, Gabriel.

              If you can conceive of an alternative, please propose it.

              Show
              salvetore Michael de Raadt added a comment - Thanks for reporting this, Gabriel. If you can conceive of an alternative, please propose it.
              Hide
              poltawski Dan Poltawski added a comment -

              IIRC marina implemnted this, so assigning to her to look at

              Show
              poltawski Dan Poltawski added a comment - IIRC marina implemnted this, so assigning to her to look at
              Hide
              marina Marina Glancy added a comment -

              The function was taken from http://www.php.net/manual/en/function.json-encode.php#78688
              I can see now that it is not working correctly.
              Will fix it ASAP.

              Show
              marina Marina Glancy added a comment - The function was taken from http://www.php.net/manual/en/function.json-encode.php#78688 I can see now that it is not working correctly. Will fix it ASAP.
              Hide
              brodock Gabriel Mazetto added a comment -

              As it's taking more time then expected, I suggest to just wipe out the cycle checking... I'm pretty sure we are not using any graph that could potentially have a cycle on it.

              Show
              brodock Gabriel Mazetto added a comment - As it's taking more time then expected, I suggest to just wipe out the cycle checking... I'm pretty sure we are not using any graph that could potentially have a cycle on it.
              Hide
              bushido Mark Nielsen added a comment -

              I second Gabriel

              Show
              bushido Mark Nielsen added a comment - I second Gabriel
              Hide
              marina Marina Glancy added a comment -

              removed anti-cycling algorithm because it is not likely to happen and if it does, this is the 3rd party plugin developer's bug anyway

              Show
              marina Marina Glancy added a comment - removed anti-cycling algorithm because it is not likely to happen and if it does, this is the 3rd party plugin developer's bug anyway
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              poltawski Dan Poltawski added a comment -

              Hi Marina,

              I agree with your pragmatic solution.

              But, I would really love this issue more if we could add some basic unit tests for it (phpunit only). It would be more robust a test than the testing instructions too.
              Are you able to create some simple tests for this?

              thanks
              Dan

              Show
              poltawski Dan Poltawski added a comment - Hi Marina, I agree with your pragmatic solution. But, I would really love this issue more if we could add some basic unit tests for it (phpunit only). It would be more robust a test than the testing instructions too. Are you able to create some simple tests for this? thanks Dan
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              brodock Gabriel Mazetto added a comment -

              I suggest to test just the behaviour of the convert_to_array function. I really believe that testing filepicker is out of scope of this specific ticket. It could be an improvement to the original ticket, but it should not be a blocking to get this one fixed, as it's related to a lot more things than just filepicker.

              Show
              brodock Gabriel Mazetto added a comment - I suggest to test just the behaviour of the convert_to_array function. I really believe that testing filepicker is out of scope of this specific ticket. It could be an improvement to the original ticket, but it should not be a blocking to get this one fixed, as it's related to a lot more things than just filepicker.
              Hide
              poltawski Dan Poltawski added a comment -

              Agreed, simple unit tests for convert_to_array() was what I was asking for.

              Show
              poltawski Dan Poltawski added a comment - Agreed, simple unit tests for convert_to_array() was what I was asking for.
              Hide
              marina Marina Glancy added a comment -

              added test for 2.3 and master

              Show
              marina Marina Glancy added a comment - added test for 2.3 and master
              Hide
              poltawski Dan Poltawski added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks a lot Marina, i've integrated that now.

              Show
              poltawski Dan Poltawski added a comment - Thanks a lot Marina, i've integrated that now.
              Hide
              phalacee Jason Fowler added a comment -

              Tests passed Marina

              Show
              phalacee Jason Fowler added a comment - Tests passed Marina
              Hide
              markn Mark Nelson added a comment -

              Was taken to the root folder for the course every time I clicked on the link 'Server Files'. Passed.

              Show
              markn Mark Nelson added a comment - Was taken to the root folder for the course every time I clicked on the link 'Server Files'. Passed.
              Hide
              poltawski Dan Poltawski added a comment -

              Congratulations, you've done it!

              Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

              Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

              Show
              poltawski Dan Poltawski added a comment - Congratulations, you've done it! Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

                People

                • Votes:
                  1 Vote for this issue
                  Watchers:
                  2 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    12/Nov/12