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
    • Rank:
      43590

      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;
      }
      

        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: