Guard against broken behaviors

If this works (should be fairly easy to test) I'm totally for this.

Works great in latest Chrome and Firefox (needs IE test), one thing though:

+++ b/core/misc/drupal.jsundefined
@@ -103,13 +113,23 @@ Drupal.detachBehaviors = function (context, settings, trigger) {
+  if (errors.length) {
+    throw errors.join(', ');
+  }

Shouldn't this be a loop on errors? Merging them into a single string make it difficult to process them later, e.g.

for(i in errors) {
  throw errors[i];
}

All good for me, but it needs some other browser tests (dear IE)

This sort of error handling is a must!

will definitely test if I find a moment to do so.

Are there any places where we could make use of this? Maybe on the AJAX errors? I know up until recently, it just gave you a big ol' message box error.

[EDIT] Added the issue as a follow up in the issue summary.

+++ b/core/misc/drupal.js
@@ -10,6 +10,28 @@ jQuery.noConflict();
 /**
+ * Custom Error thrown after attac/detach if one or more bebavior failed.
+ *
+ * @param list
+ *   An array of errors thrown during attach/detach
+ * @param event
+ *   attach or detach
+ */

1) Typo in "attac"

2) Typo in "bebavior" (should also be plural, I think)

3) Missing final sentence period in list param description.

4) Any chance to make the event param description less cryptic? :)

5) It would be great to clarify how this Error "behaves" for developers; e.g., is it output as an alert? In the console? Does the behavior depend on the browser? Does it depend on the error(s)? Lots of unknowns here. :)

+++ b/core/misc/drupal.js
@@ -10,6 +10,28 @@ jQuery.noConflict();
+  // Pretty print the list of errors.
+  var messageList = [];
+  messageList.push(this.event);
+  for (var i = 0, il = this.list.length; i < il; i++) {
+    messageList.push(this.list[i].behavior + ': ' + this.list[i].error.message);
+  }
+  this.message = messageList.join(' ; ');

1) It's not clear to me why we're building a second stack of messages and joining that in the end, instead of producing the pretty output within that loop directly?

2) Speaking of pretty, any chance to inject newlines between the messages (as opposed to that " ; ", which I guess doesn't really produce "pretty" output)? [Depends on the final output area/context, I guess...]

+++ b/core/misc/drupal.js
@@ -10,6 +10,28 @@ jQuery.noConflict();
+DrupalBehaviorError.prototype = new Error();

Is it proper to instantiate a new Error for the prototype (as opposed to just referencing it)?

+++ b/core/misc/drupal.js
@@ -103,13 +135,23 @@ Drupal.detachBehaviors = function (context, settings, trigger) {
+    throw new DrupalBehaviorError(errors, 'detach:' + trigger);

A dot/period between detach and trigger might make more sense?

Tested this on my local and it did started throwing me an error caused by the admin overlay wich wasn't caused by the patch but by a badly installed Drupal :p

But it was a perfect test, so the patch worked fine :)

(also did some other js errors including the example from comment #3)

I wrote a quick Testswarm test for this patch:

/**
 * Tests Drupal.attachBehaviors() with a failing behavior. See #1639012
 */
Drupal.tests.drupalBehaviorsGuard = {
  getInfo: function() {
    return {
      name: 'Drupal.attachBehaviors() Guard',
      description: 'Tests for Drupal.attachBehaviors() with behaviors that throw errors.',
      group: 'System'
    };
  },
  setup: function() {
    this.originalBehaviors = Drupal.behaviors;
    Drupal.behaviors = {
      undefinedVar: {
        attach: function () {
          console.log("this is bad, I'm going to fail");
          FAIL++;
        }
      },
      throwError: {
        attach: function () {
          console.log('This is reaaaally bad');
          throw new Error("Oups");
        }
      },
      validBehavior: {
        attach: function () {
          // Just put an ok statement here. If previous behaviors fail, and there is no guard
          // this behavior will not run, and Qunit will complain: "Expected 1 assertions, but 0 were run".
          ok(true, Drupal.t('Behavior ran after failing behaviors.'));
        }
      }
    };
  },
  teardown: function() {
    Drupal.behaviors = this.originalBehaviors;
  },
  tests: {
    behavioursGuard: function ($, Drupal, window, document, undefined) {
      return function() {
        expect(1);
        // Test attaching failing behaviors.
        try {
          Drupal.attachBehaviors('Attach context 1', 'Attach settings 1');
        }
        catch(e) {
          // Catch the errors after all behaviors have executed, or the test will fail anyway.
        }
      }
    }
  }
};

(see 855a099)

Before patch:

After patch:

+++ b/core/misc/drupal.jsundefined
@@ -10,6 +10,28 @@ jQuery.noConflict();
+ * Custom error type thrown after attach/detach if one or more bebavior failed.

Fixed typo.
Not a grammar guru, but singular sounds wrong, changed it to a plural.

+++ b/core/misc/drupal.jsundefined
@@ -10,6 +10,28 @@ jQuery.noConflict();
+ *   The 'attach' or 'detach' string.

Sounds kinda weird, changed it to "A string containing either 'attach' or 'detach'."

Didn't test functionality as the QUnit test in #16 covered that rather well.

So I guess we just need sun to review his objections?

Patch looks sound to me, only comments have been changed since #14, so tests from #16 still apply.

Has my blessing.

Damn wireless double post.

Also looks good to me :)

Committed to 8.x. Thanks.

...according to #11 this can be backported to D7. Right?

Lovely, thanks everyone :)

I'd suggest to keep D7 alone... this change would introduce a "sudden" difference (sorta API change) with regard to how errors are handled in Drupal behaviors, which might produce some unexpected havoc. Although that's just my feeling, feel free to disagree. ;)

Given #1776030: Machine name JavaScript behavior is broken, since jquery.once isn't loaded, in hindsight, I'm not sure whether this was a good idea... ;)

For almost one entire week, no one seems to have noticed that the machine name behavior is completely broken in HEAD... Only by knowing that "something" is missing on a page and opening the JS console, (potentially tons of) errors are revealed.

Don't get me wrong, I really liked this idea, because I regularly have to deal with lots of bogus JS... but now that I experienced the consequences of this change, it really has a huge touch of "babysitting broken code".

So I wonder whether this is actually a good idea, or whether we shouldn't revert it... :-/

What do you think?

I don't think this should be reverted. Currently there's a lot of work going on in Drupal JS, but once the core JS is stable again, this is an absolutely great patch. We have some javascript-heavy modules (Clientside Validation in particular) and we get a lot of people saying "it doesn't work", when after a lot of debugging, the cause lies with an other js file throwing errors. Right now we just need to test a bit more before committing a js patch, but once again, once js is stable again, I think this is an excellent patch.

I do see your point sun, but I have to agree with Jelle's reasoning.

Also note that this wouldn't be a problem at all if we had tests for JS.

But at least that gives you the power to shoot down js developers and force them to fix it :p

I'd really like to see this reverted.
To properly debug an error I had, I had to revert this patch just to be able to find it.

This removes the try/catch while retaining the improvement of the for loop.

Re-opening again after #1879240: Cannot add new filter group.

In the views issue, in HEAD, here is the message in the console:

Uncaught DrupalBehaviorError: attach ; viewsUiRearrangeFilter: i is not defined drupal.js:93
Drupal.attachBehaviors drupal.js:93
Drupal.ajax.commands.viewsSetForm ajax.js:16
Drupal.ajax.success ajax.js:440
ajax.options.success ajax.js:202
fire jquery.js:974
self.fireWith jquery.js:1082
done jquery.js:7788
callback jquery.js:8500

With the patch in #37:

Uncaught ReferenceError: i is not defined views-admin.js:569
Drupal.viewsUi.rearrangeFilterHandler.insertAddRemoveFilterGroupLinks views-admin.js:569
Drupal.viewsUi.rearrangeFilterHandler views-admin.js:511
Drupal.behaviors.viewsUiRearrangeFilter.attach views-admin.js:484
Drupal.attachBehaviors drupal.js:60
Drupal.ajax.commands.viewsSetForm ajax.js:16
Drupal.ajax.success ajax.js:440
ajax.options.success ajax.js:202
fire jquery.js:974
self.fireWith jquery.js:1082
done jquery.js:7788
callback jquery.js:8500

The actual problem was in views-admin.js, around line 569. Which is what the second error message told me. The first, from HEAD, told me where the exception was caught, which was useless.

I'm on Chrome 24, the latest version. Saying that FF works doesn't really help me. I'm pretty sure Chrome has a bigger marketshare at this point as well.

I RTBC'd the devel issue, this is a great compromise. Thanks nod_!

Why doesn't the JS error reporting correspond to the PHP error reporting?

We have a dedicated configuration UI to manage error reporting. We should re-use it.

There.

You're referring to:

Hiding errors != logging errors

What you're missing is that an error reporting config that tells the app to REPORT errors actually reports errors and doesn't hide them away in log messages.

Logging errors != errors breaking the app.

Just logging was the original intention of this issue. But unconditionally hiding away errors causes actual errors to go unnoticed.

Whether still logging errors when errors should be hidden is legit, is a slightly different question. Once you introduce logging, new responsibilities for you. ;)

What matters for developers is that error reporting works in the way they expect it to work. Mirroring the PHP error reporting settings into JavaScript seems to be the most sensible compromise we can find for this issue, in order to make all sides happy.

The patch in #49 makes sure to not output an additional JS setting when error reporting is hidden; i.e., bandwidth is saved in production.

Now where does that leaves me knowing I explicitly checked: Errors message to display: None.

Um. There: (emphasis mine)

Works exactly like it should for me. Steps to reproduce:

1. Apply attached patch (same as before, just with additional error in text.js).
2. Visit /node/add
3. Hard-reload the page to refresh the browser JS cache.
4. Depending on your error reporting configuration on /admin/config/development/logging (which you can open in a separate tab):
   • if set to hide errors, all behaviors except of text.js are working in the node form.
     (e.g., vertical tabs appear correctly, errors are hidden away and silently ignored)
   • if set to anything else, all JS behaviors that would be executed after text.js are broken in the node form.

Sorry to reopen this, but debugging Drupal behaviors has become a *** pain.

Although it seems a good idea to catch-rethrow, (I agree that adds robustness to our JavaScript execution). It's a pain.
I'm sure many developers will get confused, when they see errors in their console, pointing to drupal.js (it's there where the exception is re-thrown, but not where the error is)

I tested in Chrome DevTools, FireFox DevTools,Safari, IE11.. all of them points to "drupal.js" as the culprit. The *only* tool that shows correctly the source of the error was Firebug.

Fail-fast approach has its advantages too. We have done it like that before in Drupal 7. And Even in JQuery they don't try-catch the $.deferreds or not even the $(document).ready stack of functions. If something fails, better to know as soon as possible.

But then I saw how React library is approaching a similar problem:
https://github.com/facebook/react/blob/master/src/utils/Transaction.js#L...

It uses try-finally to let bubble up the first error, and then executes the rest of the callbacks silently in the background.
It's quite clever way of handling it, and probably a good compromise.

Patch included following this approach. And gifs before and after the patch.
Tested in IE9 too.
EDIT: The patch needs a bit more work. behaviors_done should shouldn't be outside of the scope. It's problematic when a behavior calls attachBehaviors.

Thanks, @corbacho. I wasn't aware that this issue was prematurely closed (again).

That approach looks indeed very interesting; seemingly resolves the original goal in a better way (on the assumption that it actually works).

Thanks.
I attached a working patch. I needed to add a third argument to Drupal.attachBehaviors to make the recursion works, but I found here and there strange side-effects, specially when Drupal.attachBehaviors is called inside of a behavior, that behavior won't continue executing. And, even is a cool trick code is getting complex and hairy... recursion makes everything more difficult than it should be.

Luckily I found an alternative approach that I will put in comment #62
Anyway, working patch attached for future reference.

Calling "throw" asynchronously with setTimeout, it won't halt the behaviors execution.
Patch attached.

Much more simple code, the console shows correct error information and all behaviors are executed. I like this approach. Re-throwing exceptions (instead of silenc'ing them) also makes possible to use tools like TraceKit, that captures the exceptions at a global level.

The only thing that doesn't work very well is the blue icon-button in Chrome Debugger for "Pause on exceptions" , (it will pause on the throw). The react way (patch #61) makes it work, but it's overkill solution, too complex. I don't think the "Pause on exceptions" feature is that important, in the meanwhile the console info and links there are correct.

@nod_ I'm using Object.keys. It's supported in modern browsers and we can save the "hasOwnProperty" check too. Good idea, but more work for the IE8 module

I like #63 patch. I tested and works ok. It's a good idea to name the (previously anonymous) function "runBehavior". This gives more info in the callstack.

I don't know why we need to collect the errors, we could throw them on the fly, setTimeout is taking care already of "grouping" the information after attachBehaviors is executed (since the attachBehaviors loop is synchronous). And it will make code a bit more readable.

Do you think is a good idea to make public the encapsulated function "throwError" ? For example, defined in Drupal.throwError, so it can be reused by detachBehaviors ? And the second benefit is that it could be overridden if someone needs to capture the errors for example. Or maybe devel module could re-define it for some purpose.

true, It's cleaner to read than setTimeout.
This is patch #63 with a Drupal.throwError helper to DRY'it up.
I don't know how useful is to expose the actual errors array. If someone needs them programmatically, they can use the window.onerror handler to capture errors

I think we still need it.

EDITED:

Chrome (Version 44.0.2403.89 beta-m (64-bit)) works in my testing.
Chrome (Version 46.0.2460.0 canary (64-bit)) DO NOT work.
IE11 line number is always wrong in try-catch.

Can we introduce debugMode (for exception) ?

        if (drupalSettings.JSdebugMode) {
          behaviors[i].attach(context, settings);
        }
        else {
          try {
            behaviors[i].attach(context, settings);
          }
          catch (e) {
            Drupal.throwError(e);
          }
        }

Almost going to agree with you but I just noticed D8 CORE has build-in DEV MODE.

#2538274: DbUpdateController has broken mainteneance mode logic, and update.php doesn't run due to aggregated JS assets

!defined('MAINTENANCE_MODE')

In this way, I think we should provide the best workaround to users. Switching to debug in Firefox doesn't seem like an option to me. It just force developers to hack drupal.js each time.

Ouch, mis-read it as DEVELOPMENT MODE.

Attached the patch, it's not perfect but better than now.

Thumbs up!
I agree with nod_ that is better compared to what we have now

btw, I tried Chrome 44, and also Chrome version 46.0.2463.0 canary (64-bit) and it worked fine. (pointing to the error line, not to the catch)

Less code and better error reporting - what is not to like... well I guess the lack of automated tests but hey it's js :)

Committed f914ac3 and pushed to 8.0.x. Thanks!

This does still need to be fixed in the 7.x branch, doesn't it?

No. D7 has no try-catch patterns in bootstrap

Hm, there was some discussion to fix or not fix this for D7, patch in #27 was for D7, but Sun said in #28:

I'd suggest to keep D7 alone... this change would introduce a "sudden" difference (sorta API change) with regard to how errors are handled in Drupal behaviors, which might produce some unexpected havoc. Although that's just my feeling, feel free to disagree. ;)

And nod_ agreed in #29.

So I guess it won't happen for D7.

Cleaning up drupalcampfi tags.