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

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for reporting this, Gabriel.

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

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

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

            Show
            Dan Poltawski added a comment - IIRC marina implemnted this, so assigning to her to look at
            Hide
            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 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
            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
            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
            Mark Nielsen added a comment -

            I second Gabriel

            Show
            Mark Nielsen added a comment - I second Gabriel
            Hide
            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 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
            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
            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
            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
            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 added a comment -

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

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            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
            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
            Dan Poltawski added a comment -

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

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

            added test for 2.3 and master

            Show
            Marina Glancy added a comment - added test for 2.3 and master
            Hide
            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
            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
            Dan Poltawski added a comment -

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

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

            Tests passed Marina

            Show
            Jason Fowler added a comment - Tests passed Marina
            Hide
            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
            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
            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
            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:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: