Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Drupal.js has some room for improvement and one of them is a critical bug.
The critical bug is in Drupal.checkPlain. Other JS that adds properties to Object.prototype causes these added properties to leak into checkPlain.
With this patch in place Drupal should just run as expected. It changes no APIs. More details to follow.
Comment | File | Size | Author |
---|---|---|---|
drupaljs_cleanup.patch | 1.22 KB | mfer | |
Comments
Comment #1
mfer CreditAttribution: mfer commentedI explained these problems for a blog post. If I can explain them for a blog post I can submit a patch as well. Since explaining them in detail was quite long and I do not want to do it again checkout the blog post for the long explanation.
Comment #2
arlinsandbulte CreditAttribution: arlinsandbulte commentedSo... where is the blog post? A link would be nice.
Also, I disagree with pointing to a blog post to explain an issue. The explanation really belongs in the issue, for posterity's sake.
Perhaps a simple copy/paste? (probably not that simple, especially if you have screenshots or attachments)
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease use jQuery.each() in Drupal.checkPlain().
I don't see any bug here. Also, any reason not to use
arguments.slice(1)
? Using the prototype function seems wrong and lazy.Comment #4
mfer CreditAttribution: mfer commentedThe blog post is at http://engineeredweb.com/blog/10/11/fixing-drupal-javascript-problem-exa.... My goof on not posting it.
Using jQuery.each() in Drupal.checkPlain() is slower than the patch and does not really make sense. We are not iterating over DOM nodes so the nice elements it has to do this are not needed. If you look at what we would pass into jQuery.each() it would work (internally) similar to this patch but with more code and logic executed.
The arguments variable in every function is not an array. It is an object that works kind of like an array. It has a length property but not the rest of the Array objects functions. At least not before ES5 (which was recently passed and is not in most of the browsers most of the users around the world have). arguments.slice() is not a function we have can use at the moment.
Array.prototype.slice is the appropriate call to use here. If you poke around internally at the jQuery source you'll find they use it as well. It is not inappropriate to use.
What makes this critical can be highlighted in a simple example. If add the following line to JS in the page:
Anywhere Drupal.checkPlain is used "foo" will be replaced with "bar" in the strings.
Comment #5
mfer CreditAttribution: mfer commentedComment #6
mfer CreditAttribution: mfer commentedAdditionally, I'd like to point out that jQuery does not use jQuery.each() for situations like Drupal.checkPlain() within its own codebase. It uses the approach this patch takes.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe have jQuery.each(), I see no reason not to use it.
That makes zero sense. You should not apply a method of the Array prototype to something that is not an array.
Comment #8
cosmicdreams CreditAttribution: cosmicdreams commentedRead your blog post, interesting stuff there. Is anyone able to test the expected performance gain from this patch?
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe performance gain would be marginal at best. Those functions are seldom called in Javascript.
Comment #10
mfer CreditAttribution: mfer commented@consmicdreams You could use something like http://jsperf.com/
@Damien Tournoud jQuery.each() is not an end all be all for loops. It's helpful in certain circumstances. Take a look at http://api.jquery.com/each
I'll give you 3 reasons not to use it:
for ( name in object ) {
loop. By using a native JS loop we are bypassing this extra stuff we don't need.Internally within jQuery Array.prototype.slice.apply() is used on arguments to turn them into an array. It is considered an acceptable practice even by members of the ECMA committee. I even checked YUI3 and they use it internally on arguments as well. It is considered an acceptable practice.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commented@mfer, obviously, I was talking about the generic loop function:
http://api.jquery.com/jQuery.each/
This function is *designed* to be a generic iterator. It allows you to write more readable/natural Javascript code without having to worry about implementation details. jQuery.each() works exactly in the way foreach() works in PHP.
You could either use (your code):
or:
The latter is way more readable, natural and as a consequence less error prone for non-hardcore Javascript developers. The speed difference should be minimal, especially on non-naive Javascript execution engines.
It's an implementation detail of Javascript that we should try to avoid. I see that
jQuery.makeArray()
is a documented way of doing that. jQuery has a test for this exact use case:Comment #12
mfer CreditAttribution: mfer commented@Damien Tournoud jQuery('#foo').each() literally is a wrapper around jQuery.each(). See the code:
The inner complexities are still there.
But, more importantly, using jQuery.each() in your implementation keeps the bug from #4 where someone can inadvertently add properties to the source/replace object by adding them to Object.prototype. jQuery.each() does not have a filter mechanism. The code you would need reads:
Is that really more readable than:
jQuery.each() is not the end all be all of loops. If you look at jQuery internally and other code written using jQuery each() is often not used and native loops are used.
Why? JavaScript is what it is. There are some beautiful parts. Some so-so parts. And, some downright ugly parts. Using Array.prototype.slice.apply() on arguments is not one of the ugly parts. It's acceptable for people who write JavaScript to use, why not us?
jQuery.makeArray() uses Array.prototype.slice.apply() to make something an array. Then it has some logic to add results to it. If you use that you would do it in2 steps.
Why do it in 2 steps when you can do it in 1 which is faster, uses less logic, and is common enough is JS circles?
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, that's reasonable. I incorrectly thought (based on the examples on the documentation) that jQuery.each() already filtered the indirect properties, but it is not actually the case.
I'm still not convinced about the second change. At the minimum we need a comment to explain why this is done. I know that documenting code is not "common enough in JS circles", but that doesn't mean we should do that :)
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedI downgraded this above. Nothing is broken here, except if another script pushes stuff in the main object constructor. This is sadly "common enough in JS circles" but clearly downright ugly.
Comment #15
ksenzeeThere's no reason not to borrow a useful method from the Array prototype. JS uses prototypical OO, where methods are first-class objects and can easily and reasonably be reused, not classic OO, where using a method from an unrelated class would be insane. And mfer is exactly right: there are excellent reasons for avoiding jQuery.each. It's especially pernicious if you use it with an inline closure and not a named function. All of this is a Drupal 8 discussion, which I am very much looking forward to having, but a Drupal 7 critical issue is definitely not the place for it. Let's keep jQuery out of this as much as we possibly can, and stick to straight JS.
Comment #16
ksenzeex-post.
Comment #17
mikl CreditAttribution: mikl commentedWow, nice catch, mfer. I had no idea we had such poor quality JavaScript in core.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's add some comments in both places.
Comment #19
mfer CreditAttribution: mfer commentedWhile I'm a fan of documentation I'm trying to understand to what level we need to document. The two items I see to document (hasOwnProperty and Array slice usages) are both basics of using the language. They are not fancy tricks or anything out of the ordinary in JavaScript. Are we to document how the language works and what we can do with it when it comes to JavaScript? What do I assume about the developers reading the code here?
My general assumption when it comes to inline documentation is to document what people who know the language might need some guidance on to understand. Holding to that I see no need for documentation here.
Comment #20
Crell CreditAttribution: Crell commentedBy and large I agree with mfer here. We don't document PHP usage unless we're dealing with a language WTF or really obscure dark recesses of the language or libraries. We assume that people can and have read the PHP manual, even when we're doing something weird (like, say, hooks). For JS, we should do the same.
So the question is, do either of these changes qualify as "a language WTF or really obscure dark recesses of the language or libraries"? The slice.apply line definitely doesn't. It's basically the same concept as array_map(), at least that's how my PHP brain is interpreting it.
The hasOwnProperty() issue I didn't know about either, although now that Matt has described it I get why it exists. That's only a language WTF to someone who doesn't understand prototypical inheritance, which is most Drupal developers but no serious JS developers. Arguably we could document that as an excuse for a "teachable moment", but since we don't do that for any of our PHP OO (we assume that people reading OO Drupal PHP code understand interfaces, abstract classes, final, etc.) I don't know that it's really needed here either.
Comment #21
mfer CreditAttribution: mfer commented@Crell, thanks for explaining what I was thinking better than I did. :)
Comment #22
mfer CreditAttribution: mfer commentedSince mikl put this to rtbc and it was switched to needs work to discuss comments I'm putting it back now that (I think) that discussion is done.
Comment #23
mikl CreditAttribution: mikl commented#20: Agreed, I think there’s little here that needs documentation. Googling for “hasownproperty foreach” tells you just about all there is to know :)
Comment #24
Dries CreditAttribution: Dries commentedDamZ, you're good or?
Comment #25
mfer CreditAttribution: mfer commentedAny word on this? I don't want it to fall through the cracks.
Comment #26
webchickCrell and I talked about this patch, he explained to me that there's a potential security issue here, so I decided to commit it since the thing this was being held up for was documentation.
However, I think that documentation ought to be discussed in a separate issue, because this is not the only weird part of the code right now. Also, nothing here sticks out at me as a "someone is going to try and 'fix' this later" type of problem.
Committed to HEAD. Thanks!
Comment #27
mfer CreditAttribution: mfer commented@webchick - The issues highlighted here should be documented somewhere. I think some sort of guide on how to write good JS would be useful.