Namespace the Drupal jQuery functions

Thanks for opening this Rob Loach. Besides the new .once() method (which isn't yet committed), we should change the already committed Vertical Tabs methods to use this convention (as per your example). #323112: Vertical Tabs

$().Drupal.stuffing() things shouldn't be too bad and maybe not that big of a job. I think modules could add their functions easily given a standard way of adding them without being a requirement or breaking any module's javascript. We might be able to protect drupal jquery functions a lot better also.

At first I didn't like $().Drupal.* much (I prefered lowercase "drupal"), but after thinking about it for a moment, I realized we use a capital D for JavaScript settings, like Drupal.settings.myModule. So since we've already got one object "Drupal", it'd be hella confusing to have another object "drupal" in a different place.

So $().Drupal.methodName() gets my vote.

As mentioned in #444344: jQuery .once() method for adding behaviors once, $(...).Drupal.once() doesn’t work. See http://drupal.org/node/444344#comment-1520596 for some options.

As @kkaefer noted, on #444344: jQuery .once() method for adding behaviors once, $().Drupal.* won't work because of how JavaScript handles this. Instead of this pointing back to the jQuery object it would reference the Drupal object. So, things like this.each() wouldn't work in the drupal methods. Not only does it kill chaining... it's a pain in the rear to work with.

I agree with the rest of you that the next best thing is to prepend drupal to the methods to end up with things like drupalSetSummary(), drupalGetSummary(). Camel Case is already the JS method naming convention used in core.

Ok, I think the solution is pretty simple. Document Drupal and make it use jquery modules directly. Plain and simple. I'll post a few things to back this up, but I think we should put more effort in making api.drupal.org parse at least core js files for doc (then or at the same time all modules..) and have something clear about how Drupal handles javascript, which is the jquery way right? And while we're there give a radio button to select which jquery version they want. So... I'll stop talking and get to work in a week or two, or gladly review / help / comment on anything that comes up ;)

Found an instance of:

- $.fn.getSummary = function () {
+ $.fn.drupalSetSummary = function () {

Replacing a get call with a set call... not so good. This naming slip is in form.js, collapse.js, and vertical-tabs.js.

Didn't apply the patch, but the changes look good and I'm ok with changing it to .drupal*

+1

Also requesting subscribers here to have a look at #674716: We need guidelines on usage of DOM events and jQuery's namespaced events

RTBC?

Reroll

Committed to CVS HEAD. Thanks.

Looks like we missed one last occurrence from comment-node-form.js, and also I've added check to node.js to make sure the function exists (see #752272: node.js uses setSummary even when it is not available.).

I had a look at this patch, but it doesn't replace all occurrences of setSummary with drupalSetSummary. I believe this issue should be set back to fixed, and work should continue at #770312: Calls to setSummary() must be renamed to drupalSetSummary(), which has a working patch that fixes the problem. I think #752272: node.js uses setSummary even when it is not available. should be marked duplicate as its patch is inferior to both this patch and the linked patch.

I've decided to merge those two other issues into this one.

From #752272: node.js uses setSummary even when it is not available.:

In some cases, when the node form for example doesn't contain any fieldsets, the setSummary function is not available, and due to this node.js's code fails. There is no check if the function exists as in some other JavaScript files.

I'm confused about the necessity of checking for the function's presence.

Consider: drupalSetSummary is defined in form.js, which is included on the page by form_process_fieldset(), which is called whenever a fieldset is part of a form on the page.

All of the calls that are affected in this patch seem to be implicitly only there when a fieldset is present:

1. book.js is attached to a particular fieldset by book.module.

2. The same applies to comment-node-form.js, which is also attached to a particular fieldset by comment.module.

3. For filter.admin.js, theoretically it is possible that no fieldsets are rendered when no filter provides any filter settings subform, while filter.admin.js will still be included. In practice this is not posible since the core module defines several filters that provide settings, and these will always be rendered (even though they may be invisible, but that doesn't affect the inclusion of form.js by form_process_fieldset).

4. Even for node.js, the case where the bug was apparently noticed, node.js is attached to the "revision_information", "author" and "options" fieldsets. See point 1 and 2, implicitly when node.js is there, form.js is supposed to be there too.

=> I can't see a case where one of these four Javascript files would be included on the page while form.js is not included, and thus drupalSetSummary would not be defined. Is there an error in my reasoning?

I've messaged the user who reported drupalSetSummary sometimes being undefined.

In any case, we should at least have a patch here that replaces occurrences of setSummary with drupalSetSummary.

I can roll that, but I want to see if the original reporter can reproduce the error.

The patch committed in #17 missed fixing four places where setSummary() needed to be renamed to drupalSetSummary().

Here's is a patch that corrects that by simply renaming the function in these four places.

Let's get fix bug fix committed. If Poetro comes back to explain how it's possible for drupalSetSummary to be undefined, we can re-open this issue to address that bug. However, as Arancaytar analyzed in #24, it doesn't look like it's (currently) possible for drupalSetSummary to be undefined in situations where it is called. Maybe the bug reported by Poetro has since been fixed by other commits.

In some cases misc/form.js is not loaded then the drupalSetSummary will be undefined. It can be not loaded for various causes, while for example the node.js is loaded.
The misc/form.js is loaded in case of fieldset preprocess, user and node admin pages.
The modules/node/node.js is loaded on node forms for the fieldsets. In case of some multistep node forms the node.js is loaded, while the form.js is not. I'll dig deeper into it, if the problem still exists, maybe the recent changes solved the usage of node.js, and are strictly added in case of the fieldset elements, as it is currently the case.

I've checked on the issue, and found the following in one of the node forms we developed:

    // Remove other node options, keep Status.
    $form['options']['#type'] = 'container';
    foreach (element_children($form['options']) as $key) {
      if ($key != 'status') {
        $form['options'][$key]['#access'] = FALSE;
      }
    }

This way the node.js is still added, as it is added to the form element $form['options'], but the fieldset preprocess doesn't happen, as there are no other fieldsets in this form, as they are moved to another step, or not fieldsets. Due to this, i don't know if it is core's fault, or our fault.

Thanks for the update! Setting back to CNR.

Frankly I don't see what this has to do with fixing the obvious error that four instances of setSummary() were not renamed to drupalSetSummary(), breaking the core JavaScript. Regardless of whether anyone can confirm or track down the problem stated in #28 and #29, these four instances need to be fixed. So let's do it.

Right now, the core JavaScript throws errors, interfering with other JavaScript that might be loaded. In fact, let me repeat what Arancaytar said in http://drupal.org/node/770312#comment-2842040 : "...upgrading to critical since javascript execution is broken by this and it affects several modules."

TR is right; the two are separate issues - one is a trivial clean-up of the incomplete patch earlier committed here, and the other seems to be a fairly tricky multistep bug, which might have other repercussions or require a testcase further on.

So I set #752272: node.js uses setSummary even when it is not available. back to CNW.

My patch in #770312: Calls to setSummary() must be renamed to drupalSetSummary() (TR's patch in #26 here) seems the right one for this issue.

Ok then, back to RTBC if we are going to solve #752272: node.js uses setSummary even when it is not available. separately.

Committed to CVS HEAD. Thanks.