Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The 'killswitch' in JavaScript is a pre-jQuery relic. If the browser doesn't support the features we query for, the jQuery will fail horribly anyway. The percentage of browsers not supporting jQuery (but supporting JavaScript) is too low to really be taken serious.
Comment | File | Size | Author |
---|---|---|---|
killswitch-1.patch | 1.93 KB | kkaefer | |
Comments
Comment #1
kkaefer CreditAttribution: kkaefer commentedComment #2
Dries CreditAttribution: Dries commentedLooks good to me but it would be good to have a JavaScript wizard confirm this too.
Comment #3
jjeff CreditAttribution: jjeff commentedLooks good to me. I'm all for getting rid of the killswitch.
However, we'll need to be sure to add this removal to the module/theme upgrade documentation.
Also, kkaefer, what's going on with the document.cookie stuff in there? Are we moving from Drupal.settings over to a cookie-based system?
Other than the cookie question, this looks good to me.
Comment #4
markus_petrux CreditAttribution: markus_petrux commentedThe cookie is created without explicit domain, that means the cookie will be created under the current domain in the URL. It looks good, but there's an issue here that I would like to mention now that this snippet is being review.
The cookie domain in settings.php can be set to a higher level of the domain being used for the site, and that allows sharing the cookies between subdomains of the same domain. But this cookie "has_js" cannot be shared because it will exist a different version for each subdomain.
If the global $cookie_domain was in Drupal.settings, then it could be used here to create this cookie under the same exact scope the rest of the cookies are created by Drupal.
Comment #5
kkaefer CreditAttribution: kkaefer commentedThis patch is not about the cookie domain at all and doesn't change anything about that. It has been there before and it is still there.
Comment #6
dmitrig01 CreditAttribution: dmitrig01 commentedCode looks good and I tried it and everything works as expected
Comment #7
quicksketchYep, it is time, sorry IE4. It's nice that the introduction of behaviors made this patch so much smaller that it would've been in D5 days. Second this RTBC.
Comment #8
webchickGreat, thanks for all the reviews folks! I've committed this to HEAD. Marking needs work, as this needs to be reflected in the upgrade documentation, as I'm pretty sure checking for Drupal.jsEnabled is considered a best practice in contrib js files.
Also, markus_petrux's comment at #4 sounds like it's worthy of further discussion. I didn't block this patch for that since kkaefer is right, and it's just moving existing lines of code around, but Markus, could you start another issue to discuss moving this to the Drupal.settings array?
Comment #9
webchickComment #10
markus_petrux CreditAttribution: markus_petrux commented@webchick #8: #445098: Provide a consistent method to generate cookies
I hope that helps.
Comment #11
jhodgdonI'm assigning this to myself to add it to the module/theme upgrade guides
Comment #12
jhodgdonI've added information to:
* Module upgrade guide http://drupal.org/node/224333#no-jsenabled
* Theme upgrade guide (same text) http://drupal.org/update/theme/6/7#no-jsenabled
* Categorical module upgrade page http://drupal.org/node/394070 (I marked it as "unknown" as to Coder warning/upgrade status).
If someone could review the text there, that would probably be good... if the text on those pages is OK, it is probably time (I think) to mark this issue as fixed.
Comment #13
jhodgdonComment #15
jhodgdonMaybe the bot will ignore it this time...
Comment #16
dmitrig01 CreditAttribution: dmitrig01 commentedLooks good