I have noticed that any behavior code that throws an exception during attach or detach prevents subsequent behavior code from executing. This makes it difficult to trust that my behavior code will get called when combined with code from a number of other sources.
This issue is really simple to fix; simply wrap the attach and detach calls in a try/catch block. The errors will still be visible to anyone that is looking for them, but at least individual modules could depend on their behavior code getting called.
The attached patch resolves the issue, allowing all behaviors to attach and detach even if one of the behaviors has an error.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | behaviors-990880-5.patch | 743 bytes | muhleder |
| behaviors.patch | 502 bytes | paul.lovvik |
Comments
Comment #1
ksenzeeThis is definitely an improvement over the current state of affairs. We do need some way for contrib developers to figure out why their code isn't running, though. Just swallowing their exceptions will confuse the heck out of them. We should probably add a JS logging system for D8. In the meantime, what if we throw a new exception after all the behaviors have run, so people at least know there was an error?
Comment #2
ksenzeeThis needs to get fixed in D8 first now.
Comment #3
nod_#1+1 #1419652: JavaScript logging and error reporting needs reroll
try/catch slow things down, needs benchmark too.
Comment #4
muhleder commentedWhy not just
console.log(e)?The first thing I'd look at is the console if javascript isn't working, to see if there is a fatal error.
What's happening here is that a try finally in the surrounding jquery.each is catching the error allowing it to fail silently.
Comment #5
muhleder commentedOk, so I think this is a better approach, don't use jquery each so errors won't get caught, and will be reported in the console.
This would also apply to behaviors.detach(), and probably anywhere else that jquery.each is used to run unknown functions.
Comment #6
muhleder commentedComment #8
nod_umm have you checked drupal.js lately?
Also, this will not fix the bug described here.
Comment #9
muhleder commentedClearly I haven't checked drupal.js lately. I was looking at the earlier patch and didn't realise it had changed so much from Drupal 7. It looks like you've already done what this patch suggests.
I'm testing in Drupal 7 and this approach does fix part of the bug described here. Errors in contrib modules behaviors.attach methods now will report an error in the console rather than silently failing.
Surely it's fairly standard with js that any fatal error will stop all subsequent js code from running? The real problem with this bug is that there is no indication that there has been a failure. So a js bug in contrib module x silently breaks js functionality in contrib module y and z.
How about backporting your improvements from 8 into 7?
Comment #10
nod_Glad you asked, #1428524: Replace all $.each() with filtered for loop, please review :)
The issue here is that we don't want 1 contrib behavior breaking everything. Regular event listeners behave like that, our brittle callback system doesn't (and jQuery events listener don't either for that matter).
Comment #11
nod_up to date and working patch over at: #1639012: Guard against broken behaviors