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

Andrew Nicols
added a comment - 16/Mar/13 8:52 PM 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?

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.

Andrew Nicols
added a comment - 17/Mar/13 5:08 PM 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.

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-").

Eloy Lafuente (stronk7)
added a comment - 19/Mar/13 1:40 AM 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

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).

Andrew Nicols
added a comment - 19/Mar/13 9:59 AM 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

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.

Andrew Nicols
added a comment - 19/Mar/13 11:44 PM 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.

Eloy Lafuente (stronk7)
added a comment - 20/Mar/13 12:06 AM 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!

Marina Glancy
added a comment - 21/Mar/13 3:46 AM 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

Marina Glancy
added a comment - 21/Mar/13 4:06 AM 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?

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.

Andrew Nicols
added a comment - 21/Mar/13 8:31 AM 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.

Eloy Lafuente (stronk7)
added a comment - 21/Mar/13 8:56 AM 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