Moodle
  1. Moodle
  2. MDL-38515

Undefined offset in yui_combo.php

    Details

    • Testing Instructions:
      Hide

      With this issue it's really worth testing without the patch to understand the issue.

      Dependency fix check

      • Open the JS console
      • Open any page with a TinyMCE editor (which accepts files)
        • Confirm that the editor works
        • Confirm that no JS errors were shown

      Error notices

      • In the JS console type:
        YUI().use('moodle-core_filepicker');
        
        • Confirm that an error was shown in your console (Theoretically, not all browsers actually support this though I'm not aware of any which don't)
        • Confirm that a file was loaded but had an error message as it's content
      Show
      With this issue it's really worth testing without the patch to understand the issue. Dependency fix check Open the JS console Open any page with a TinyMCE editor (which accepts files) Confirm that the editor works Confirm that no JS errors were shown Error notices In the JS console type: YUI().use('moodle-core_filepicker'); Confirm that an error was shown in your console (Theoretically, not all browsers actually support this though I'm not aware of any which don't) Confirm that a file was loaded but had an error message as it's content
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-38515-m24
    • Pull Master Branch:
    • Rank:
      48512

      Description

      David Mudrak pointed out last night that he is now seeing errors such as:

      [Sat Mar 16 19:57:53 2013] [error] [client 192.168.1.19] PHP Notice:  Undefined offset: 0 in /Users/nicols/git/software/moodle/theme/yui_combo.php on line 99, referer: http://tangerine.local/moodle/course/edit.php?id=9
      

      in his error log for yui_combo.php.

      Looking into this, it's caused by an incorrect dependency on moodle-core_filepicker from within core_filepicker.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          The dependency change part of this issue should probably be backported to 2.3 and 2.4. I don't feel that we should backport the error messages though.

          Marina, would you be able to peer review this for me?

          Show
          Andrew Nicols added a comment - The dependency change part of this issue should probably be backported to 2.3 and 2.4. I don't feel that we should backport the error messages though. Marina, would you be able to peer review this for me?
          Hide
          Andrew Nicols added a comment -

          p.s. this will need to be rebased for MDL-38391

          Show
          Andrew Nicols added a comment - p.s. this will need to be rebased for MDL-38391
          Hide
          Petr Škoda added a comment -

          Looks ok to me, I suppose this should be integrated by SamH because he might know the most about the combo loader...

          Show
          Petr Škoda added a comment - Looks ok to me, I suppose this should be integrated by SamH because he might know the most about the combo loader...
          Hide
          Andrew Nicols added a comment -

          After speaking to Petr about this, the only other person who really knows enough about the loader is Sam so he suggested that if Sam integrates this, and Marina checks the filepicker functionality that is probably the best approach to this issue.

          Show
          Andrew Nicols added a comment - After speaking to Petr about this, the only other person who really knows enough about the loader is Sam so he suggested that if Sam integrates this, and Marina checks the filepicker functionality that is probably the best approach to this issue.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Just guessing if the "!b.length" condition in "moodleConfigFn" there is enough, or it should be stricter and require also a minimum number of elements or no, like the yui_combo.php does (<=3, apart from "moodle-").

          Also, pinged Sam about this...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Just guessing if the "!b.length" condition in "moodleConfigFn" there is enough, or it should be stricter and require also a minimum number of elements or no, like the yui_combo.php does (<=3, apart from "moodle-"). Also, pinged Sam about this...ciao
          Hide
          Andrew Nicols added a comment -

          Hi Eloy,

          In the moodleConfigFn we take the module name (e.g. moodle-core-tooltip, or moodle-mod_kitchen-baking, or moodle-core_dock, or moodle-local_bakery-cake-skin) and:

          • s/^moodle-// on the module name:
            • core-tooltip'; mod_kitchen-baking; core_dock; and local_bakery-cake-skin
          • split the string into an array on the '-' character:
            • [ 'core', 'tooltip' ]; [ 'mod_kitchen', 'baking' ]; [ 'core_dock' ]; and [ 'local_bakery', 'cake', 'skin' ]
          • pop the last element off:
            • [ 'core' ]; [ 'mod_kitchen' ]; []; and [ 'local_bakery', 'cake' ]
          • therefore length is:
          • 1; 1; 0; and 2

          The !b.length test should catch the case where an incorrectly formatted module name has been supplied (e.g. moodle-core_dock) because the array has a length of 0 (which is falsey in JS).
          Valid modules (like moodle-core-tooltip and moodle-mod_kitchen-baking) are fine because their length is 1, which is a truthful value.
          Valid skin files (like moodle-local_bakery-cake-skin) are fine because their length is 2, which is also a truthful value.

          The minimum number of elements at this point is one so the truthy test on b.length should be sufficient.

          I rather wish that we had a system for minifying a JS string because it would be good to have that code expanded with comments in it during dev (wishlist).

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Eloy, In the moodleConfigFn we take the module name (e.g. moodle-core-tooltip, or moodle-mod_kitchen-baking, or moodle-core_dock, or moodle-local_bakery-cake-skin) and: s/^moodle-// on the module name: core-tooltip'; mod_kitchen-baking; core_dock; and local_bakery-cake-skin split the string into an array on the '-' character: [ 'core', 'tooltip' ]; [ 'mod_kitchen', 'baking' ]; [ 'core_dock' ]; and [ 'local_bakery', 'cake', 'skin' ] pop the last element off: [ 'core' ]; [ 'mod_kitchen' ]; []; and [ 'local_bakery', 'cake' ] therefore length is: 1; 1; 0; and 2 The !b.length test should catch the case where an incorrectly formatted module name has been supplied (e.g. moodle-core_dock) because the array has a length of 0 (which is falsey in JS). Valid modules (like moodle-core-tooltip and moodle-mod_kitchen-baking) are fine because their length is 1, which is a truthful value. Valid skin files (like moodle-local_bakery-cake-skin) are fine because their length is 2, which is also a truthful value. The minimum number of elements at this point is one so the truthy test on b.length should be sufficient. I rather wish that we had a system for minifying a JS string because it would be good to have that code expanded with comments in it during dev (wishlist). Cheers, Andrew
          Hide
          Andrew Nicols added a comment -

          Just realised that I'd forgotten to add this blocker.

          Show
          Andrew Nicols added a comment - Just realised that I'd forgotten to add this blocker.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          yeah, those long one liners are anything but readable. Thanks for the explanation.

          I think I'm going to push this now. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - yeah, those long one liners are anything but readable. Thanks for the explanation. I think I'm going to push this now. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          lol, master is, now that yui_config has landed, conflicting here.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited lol, master is, now that yui_config has landed, conflicting here.
          Hide
          Andrew Nicols added a comment -

          Just in the process of rebasing it for you.

          Show
          Andrew Nicols added a comment - Just in the process of rebasing it for you.
          Hide
          Andrew Nicols added a comment -

          Have rebased on integration/master and pushed.
          Thank goodness for git log --patch --color-words -2

          Show
          Andrew Nicols added a comment - Have rebased on integration/master and pushed. Thank goodness for git log --patch --color-words -2
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
          Hide
          Andrew Nicols added a comment -

          I think we're going to have to revert the module dependency correction part of this. I've discovered that it was being abused in such a way that I can't easily fix without some serious work.

          I'm not sure whether we should keep the warning message in the JS, though it is only shown when $CFG->debug > 0.
          Even if we do remove the JS warning message, we should probably keep the part in theme/yui_combo.php

          Basically:

          • /lib/form/filemanager.js is described in files/renderer.php as form_filemanager. It depends on core_filepicker
          • /repository/filepicker.js is described in lib/outputrequirements.php. The YUI.add() describes it as moodle-core_filepicker though

          When the filemanager loads, it tries to depend on core_filepicker which loads the file.
          Because the content of /lib/form/filemanager.js is wrapped in a YUI.add which doesn't match, the module content is not added so it's content isn't actually made available.

          This was previously working by virtue of requiring moodle-core_filepicker as a dependency to core_filepicker, thereby adding the moodle-core_filepicker.

          The only real solution to this is to rewrite lib/form/filepicker.js, repository/filepicker.js and lib/form/filemanager.js as proper YUI modules with the correct dependencies and module names.

          Show
          Andrew Nicols added a comment - I think we're going to have to revert the module dependency correction part of this. I've discovered that it was being abused in such a way that I can't easily fix without some serious work. I'm not sure whether we should keep the warning message in the JS, though it is only shown when $CFG->debug > 0. Even if we do remove the JS warning message, we should probably keep the part in theme/yui_combo.php Basically: /lib/form/filemanager.js is described in files/renderer.php as form_filemanager. It depends on core_filepicker /repository/filepicker.js is described in lib/outputrequirements.php. The YUI.add() describes it as moodle-core_filepicker though When the filemanager loads, it tries to depend on core_filepicker which loads the file. Because the content of /lib/form/filemanager.js is wrapped in a YUI.add which doesn't match, the module content is not added so it's content isn't actually made available. This was previously working by virtue of requiring moodle-core_filepicker as a dependency to core_filepicker, thereby adding the moodle-core_filepicker. The only real solution to this is to rewrite lib/form/filepicker.js, repository/filepicker.js and lib/form/filemanager.js as proper YUI modules with the correct dependencies and module names.
          Hide
          Andrew Nicols added a comment -

          By the way, the error is only seen when viewing a page with an HTML editor. It's further confused by our having two filepicker.js files :|

          Show
          Andrew Nicols added a comment - By the way, the error is only seen when viewing a page with an HTML editor. It's further confused by our having two filepicker.js files :|
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, I've reverted the "module dependencies commits in all branches":

          master: babf34d5cfc7845e8f42b850a39646bf9a9c17c3 reverted
          24_STABLE: 573571d727ce47a39160c8583c90158accd09641 reverted
          23_STABLE: e40c3f7cc48eb66737def5a7aee14ee2aa986990 reverted

          And have cleaned the config condition because it was going to be annoying developers all the time until properly fixed the filepickers:

          master: e6f94aaa11c20058d323dc1d84e17d092badbc31

          That is, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, I've reverted the "module dependencies commits in all branches": master: babf34d5cfc7845e8f42b850a39646bf9a9c17c3 reverted 24_STABLE: 573571d727ce47a39160c8583c90158accd09641 reverted 23_STABLE: e40c3f7cc48eb66737def5a7aee14ee2aa986990 reverted And have cleaned the config condition because it was going to be annoying developers all the time until properly fixed the filepickers: master: e6f94aaa11c20058d323dc1d84e17d092badbc31 That is, thanks!
          Hide
          Andrew Nicols added a comment -

          That partial revert looks spot on.
          Thanks Eloy.

          Show
          Andrew Nicols added a comment - That partial revert looks spot on. Thanks Eloy.
          Hide
          Andrew Nicols added a comment -

          Have created MDL-38561 to address the filepicker issues, and MDL-38562 to reinsert the revert JS when it is fixed.

          Show
          Andrew Nicols added a comment - Have created MDL-38561 to address the filepicker issues, and MDL-38562 to reinsert the revert JS when it is fixed.
          Hide
          Marina Glancy added a comment -

          Test passed. Editor and filepicker works on all branches and no errors are shown.

          Second part of the test does not pass - there are no error messages in this case but as far as I can understand from the discussion in comments this has been reverted

          Show
          Marina Glancy added a comment - Test passed. Editor and filepicker works on all branches and no errors are shown. Second part of the test does not pass - there are no error messages in this case but as far as I can understand from the discussion in comments this has been reverted
          Hide
          Marina Glancy added a comment -

          sorry, does not work on 2.3 on 'Edit profile' page:

          Error: Augment failed, verify dependencies.
          throw new Error("Augment failed, verify dependencies.");
          
          TypeError: YAHOO.widget.Node is undefined (repository/filepicker.js line 15)
          YAHOO.widget.Node.prototype.refreshPreviews=function(imgid,newsrc,regex)
          
          Show
          Marina Glancy added a comment - sorry, does not work on 2.3 on 'Edit profile' page: Error: Augment failed, verify dependencies. throw new Error( "Augment failed, verify dependencies." ); TypeError: YAHOO.widget.Node is undefined (repository/filepicker.js line 15) YAHOO.widget.Node.prototype.refreshPreviews=function(imgid,newsrc,regex)
          Hide
          Marina Glancy added a comment -

          first error source: /theme/yui_combo.php?2.9.0/build/yahoo/yahoo.js line 849

          Show
          Marina Glancy added a comment - first error source: /theme/yui_combo.php?2.9.0/build/yahoo/yahoo.js line 849
          Hide
          Marina Glancy added a comment -

          grrr, I refreshed the page and don't see any more errors. I have no idea - maybe there was something cached for me? Andrew, please take a look maybe we still can pass this?

          Show
          Marina Glancy added a comment - grrr, I refreshed the page and don't see any more errors. I have no idea - maybe there was something cached for me? Andrew, please take a look maybe we still can pass this?
          Hide
          Andrew Nicols added a comment -

          Hmm, well this patch was entirely reverted for 2.3 and the only other JavaScript changes this week are are to

          • theme/mymobile/javascript/custom.js
          • lib/yui/blocks/blocks.js

          The only changes to lib/outputrequirements.php are this patch being applied, and later reverted.

          I'm unable to replicate on integration/MOODLE_23_STABLE and the error message you've given points to a comment line in filepicker.js. That said, it can only be line 117 which suggests that, for some reason, your browser was unable to load the Widget dependency, and therefore unable to access Node on YAHOO.widget.

          I would guess that this is either a transitory issue with your build which was not helped by use of the older loader and non-modular systems; or an issue as you suggest with caching.

          As I say, there haven't been any relevant changes that affect this issue, so there's not really anything to actually fail.

          Show
          Andrew Nicols added a comment - Hmm, well this patch was entirely reverted for 2.3 and the only other JavaScript changes this week are are to theme/mymobile/javascript/custom.js lib/yui/blocks/blocks.js The only changes to lib/outputrequirements.php are this patch being applied, and later reverted. I'm unable to replicate on integration/MOODLE_23_STABLE and the error message you've given points to a comment line in filepicker.js. That said, it can only be line 117 which suggests that, for some reason, your browser was unable to load the Widget dependency, and therefore unable to access Node on YAHOO.widget. I would guess that this is either a transitory issue with your build which was not helped by use of the older loader and non-modular systems; or an issue as you suggest with caching. As I say, there haven't been any relevant changes that affect this issue, so there's not really anything to actually fail.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I have tried 23 here and I've failed to get any error like that. So I'm assuming it was an interim problem and passing this now.

          If anybody has anything against that, plz tell. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I have tried 23 here and I've failed to get any error like that. So I'm assuming it was an interim problem and passing this now. If anybody has anything against that, plz tell. Ciao
          Hide
          Marina Glancy added a comment -

          As I said, I've received this error couple of times and then it disappeared. Hopefully it's not at issue

          Show
          Marina Glancy added a comment - As I said, I've received this error couple of times and then it disappeared. Hopefully it's not at issue
          Hide
          Damyon Wiese added a comment -

          This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Thanks for your contributions!

          Show
          Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: