- Steps to reproduce
1) Checkout DRUPAL-6, install it with JavaScript turned off
2) Note that after such an install, index.php is a white screen of death, and rerunning the installer results in SQL query erros.
3) Delete the DB and do a fresh checkout and note that install with JS on works
- What needs to happen:
1) If JS is off, we need to detect that and disable the eye-candy progress bar.
2) The install needs to work if JS isn't enabled.
I think it's high time we stopped worrying so much about eye candy and adding features, and started doing real honest to goodness QA on Drupal releases.
Robin
Comments
Comment #1
keith.smith CreditAttribution: keith.smith commentedComment #2
cburschkaOuch. How come nobody noticed this months ago? I kept wondering whether the installer would gracefully degrade whenever I saw that progress bar, but I never actually thought to test it instead of treating it as Somebody Else's Problem. I guess so did everyone else.
Nasty bug, that.
Comment #3
cburschkaWait, wait. Drupal's batch API is fully encapsulated. It has support for falling back to JS-disabled users.
The good news is that our QA isn't as awful as the first post at face value led me to believe. The bad news is that now we don't know why the installer still breaks.
I'll do some testing.
Comment #4
cburschkaThe bug report as it is now leaves some detail to be desired.
More accurate description here:
Comment #5
keith.smith CreditAttribution: keith.smith commentedFrom past issues that I remember but have not looked for, the installer broke when:
- someone with a js-enabled browser has visited the site previously (maybe in a previous installation or something) and has the "has_js" cookie.
- then disables javascript in the browser
- and Drupal is confused because the still-present cookie suggests that javascript works when it doesn't.
I recall testing an actual non-js installation by deleting my cookies (or really, just that one) and trying it, and as I recall, it worked fine.
I'm not sure if this is the same issue, or a different one.
Comment #6
cburschkaAdding debug output shows that $_COOKIE['has_js'] is "1" even if I am using the page without Javascript enabled.
Problem identified.
Solved? Not so much.
Comment #7
keith.smith CreditAttribution: keith.smith commentedDidn't mean to change the title.
Comment #8
keith.smith CreditAttribution: keith.smith commentedArgh.
Anyway, I think the original issues about this were "won't fix" or "by design" as this was an unusual situation that should not happen so much in the wild.
Comment #9
yched CreditAttribution: yched commentedTrue, this is a 'by design'
Comment #10
cburschkaWhy? There's a simple fix.
Do
setcookie('has_js', '');
at the start of every Drupal page, then dodocument.cookie = 'has_js=1; path=/';
in the startup JS (like now).In all versions of PHP I've used (and hopefully in all there are),
setcookie
will only send a cookie header to the client, which will take effect in the next request when the client sends the new cookies. It does not affect the $_COOKIE array in the same request.This means that whenever the page loads, the has_js cookie would be off by default, and on only if Javascript is enabled. Admittedly, this is imperfect because the server still lags behind the client: The cookie will only get cleared on the next request after Javascript is disabled, and the server will only find out during the second request after that. The server assumes that Javascript is still enabled for exactly one request after the user disables it. But it's still an improvement.
Comment #11
Robin Monks CreditAttribution: Robin Monks commentedI tried this again in lynx, with no cookies, and it still fails:
Comment #12
Robin Monks CreditAttribution: Robin Monks commentedI feel the need to strangle project.module ;)
Robin
Comment #13
cburschka#11 is a different bug. The original one is clearly related to the has_js cookie and failed installation of core modules, and it doesn't appear to cause any SQL errors. #11 sounds like #210752: Javascript error in installation: Location.toString() cannot be called.
Edit: Case in point, I cannot reproduce the bug anymore after clearing cookies and leaving JS disabled for the entire installation. I don't have lynx now, but I'll test that later.
Comment #14
cburschkaOriginal bug is also global to all places where $_COOKIE['has_js'] is used.
Comment #15
Robin Monks CreditAttribution: Robin Monks commented#13 appears to be caused, at least in some cases, by disabling JS. Since lynx never had JS I can't see how this could be this issue. I'm writing in some debug lines that should display all the cookies lynx sent.
Robin
Comment #16
keith.smith CreditAttribution: keith.smith commentedThe lynx thing is maybe caused by lack of meta redirects, as in http://drupal.org/node/204374?
Comment #17
Robin Monks CreditAttribution: Robin Monks commentedArray ( )
that's a print out of
print_r($_COOKIE);
this is after the install failed and the errors displayed. I can perform a clean install with the same data entry from Firefox with JavaScript, but not from lynx.
Robin
Comment #18
Robin Monks CreditAttribution: Robin Monks commented"No I cannot. This seemed to be old cookie issue. I successfully installed drupal without JS on few browsers (FF, Safari, Opera). This is no longer critical. Problem I'm having is that simpletest does not support meta redirects. I can go around this by manually reloading proper pages. Not sure if this is worth fixing but I'll leave the issue open." -- http://drupal.org/node/204374
Yeah, I had a similar problem with simpletest but I got around it, but, this is lynx which afaik does support the redirects. Even if it *doesn't* shouldn't refreshing the page work?
Robin
Comment #19
keith.smith CreditAttribution: keith.smith commentedGoogling for "lynx meta redirect" suggests that it does not support those tags.
Comment #20
Robin Monks CreditAttribution: Robin Monks commentedOK, so, is the meta redirect required or should refreshing do the job? It seems to be that even meta redirects shouldn't be a requirement. The odd hard core admin is going to want to do a text based install.
Robin
Comment #21
Robin Monks CreditAttribution: Robin Monks commentedI can verify that a clean firefox without JS can install Drupal under the same circumstances. So, it's related directly to browsers without meta refresh.
Robin
Comment #22
cburschkaRight... I somehow recalled that it did, because I've been asked to confirm a redirect manually by lynx before. That might have been an HTTP redirect, however.
Obvious solution: Do what every website does and add a link along the lines of
Wait while you are being redirected, or click here if it's not working for some reason.
Seriously, that's standard stuff. We've managed to avoid it in Drupal so far by using HTTP redirects in form submissions, which pretty much eliminate the need for "Thanks for posting, please wait to be redirected." But in this case, it's obvious that we need it.Comment #23
Robin Monks CreditAttribution: Robin Monks commentedCan we just make people wait while the backend stuff is done? I mean, it's only about 7 seconds anyways
Robin
Comment #24
Robin Monks CreditAttribution: Robin Monks commented(I speak of delaying the page load, so, it just takes that extra 7 seconds for the submit to go through, skipping the progress bar entirely)
Comment #25
cburschka1.) No. This bug affects every place where the batch API is used, so not using it in this case doesn't actually solve the problem.
2.) $_COOKIE['has_js'] and meta-refresh are now known to be two different bugs. I opened a separate issue.
#229905: Batch API assumes client's support of meta refresh
Comment #26
Robin Monks CreditAttribution: Robin Monks commentedArancaytar++
We also know:
http://drupal.org/node/204374 is now a duplicate of http://drupal.org/node/229905
http://drupal.org/node/210752 is caused by this bug, or http://drupal.org/node/229905
Robin
Comment #27
Robin Monks CreditAttribution: Robin Monks commentedgrr
Comment #28
jbrauer CreditAttribution: jbrauer commentedThis really is a by-design issue.
The whole point of setting the cookie is that it is a faster way of knowing if the session supports Javascript.
Running a test on every page load is an expensive way to try and re-run the test on every page load. The case where this presents a problem is where somebody turns off MySQL while in a session on a site. The test that sets the cookie is itself a Javascript so once Javascript is off unsetting the cookie via javascript is impossible. The only real alternative to the cookie seems to be running a check on every page which is very inefficient. If this kind of behavior is desired a contributed module that would remove the cookie on page loads but the performance impact for what is really an edge case (Javascript on then Off while on the same site) seems hard to justify.
Comment #29
Robin Monks CreditAttribution: Robin Monks commentedAgreed. My original blocking bug is now expressed in http://drupal.org/node/229905
Thanks all!
Comment #30
cburschkaEr, sorry?
If you switch Javascript off globally, then it will be off on every page you load, including on the Drupal site which tested true for Javascript a while earlier. This is not an edge case at all, people do switch Javascript off temporarily. As evidenced by at least 2, possibly more separate bug reports (in which the installer mysteriously failed), people keep getting this.
Serving any page that requires Javascript to be enabled is a minor design error in the first place. Indefinitely continuing to serve pages that require Javascript based on a single test for Javascript earlier in the session is bad.
The ideal way to implement graceful degradation and accessibility is to ensure that the page falls back to non-Javascript functionality without requiring different server output at all, so the server does not need to rely on any past information. Re-checking Javascript on every page is just a workaround for this. I also don't get the performance bit - setting a cookie in Javascript is a single operation, and the code indicates that it happens every time anyway, regardless of whether the cookie is already set. How would this save any performance, let alone enough to justify a critical bug?
Comment #31
cburschkaOn second thought, perhaps we can compromise differently. The $_COOKIE['has_js'] does not need to be constantly up to date provided that it isn't relied on for serving Javascript-dependent pages anywhere.
The batch API would have to serve its js-enabled page in such a way that if it is served in spite of js being disabled, it will fall back to non-js behavior.
Seriously, if the server has to know whether the client will execute Javascript as it generates the page, accessibility is not implemented properly.
Comment #32
lil-1 CreditAttribution: lil-1 commentedExcuse me! I probably post in wrong issue...
I have # 11 today installing the site-2 (multi-sites) while the first installation went perfect.
D6.1
Linux
PHP 5.2.4
Apache 1.3.39
MySQL 4.1.22
(in case)
the tricks with js don't help (though I may execute them some wrong way. I'm not sure because of multi-sites).
what issue should i subscribe now for possible solution - this one or #210752: "Javascript error in installation: Location.toString() cannot be called"?
Thank you in advance! I've been with Drupal for two weeks or so and am not sure about the rules and traditions..
Comment #33
jbrauer CreditAttribution: jbrauer commentedMoving this to Drupal 7. If left in Drupal 6 it certainly won't be changed. We'll decide in Drupal 7 if it's a bug or design item.
Comment #34
Everett Zufelt CreditAttribution: Everett Zufelt commentedJust want to add that Javascript is no longer a major accessibility concern as of WCAG 2.0. However, I still would be concerned if any Drupal pages required JS to be enabled for functionality.
A reverse paradigm to graceful degradation is progressive enhancement. User interfaces should be designed to work without JS and then progressively enhanced to provide a better experience for those who have JS enabled in their browsers. There are a lot of people, who cannot run JS in their browsers.
Comment #35
jbrauer CreditAttribution: jbrauer commentedComment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedI ran into this bug. Figured this out by accident when I turned off JavaScript to visit a questionable site and then went to install HEAD again.
It would be nice for it to fallback gracefully or provide a notice.
Comment #37
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is a bad idea anyway, and the Batch API sending two different HTML based on whether the browser supports cookie or not is a design flaw. Let's kill this cookie and fix the Batch API in Drupal 8.
Comment #38
yched CreditAttribution: yched commentedThe "different HTML based on a cookie" approach was never deemed ideal - however, no other solution was found back then (and since then).
IIRC, the challenge is that the non-js mode relies on a redirect HTTP header. And AFAIK there's no way to 'cancel' this redirect on the client side in favor of an ajax call if js is enabled.
Comment #39
Damien Tournoud CreditAttribution: Damien Tournoud commented@yched: isn't the nojs version based on HTML meta redirect headers? Those are part of the DOM and can be removed or altered.
Also, it seems that
<noscript>
is valid inside<head>
, which means we can probably wrap the<meta>
header with it.Comment #40
yched CreditAttribution: yched commentedDoh, you're right, I should have actually checked before posting :-p.
not according to http://www.velocityreviews.com/forums/t160868-noscript-trap-redirection-...
Removing meta tags from the dom with JS:
Hm, maybe - wouldn't it be too late, though ? Would need to be tested across browsers, I guess.
Comment #41
yched CreditAttribution: yched commentedAccording to http://www.sitepoint.com/forums/showthread.php?t=575466, removing meta tag from the dom doesn't work. The element gets removed, but redirection still happens.
It seem that
<noscript><meta /></noscript>
does work - http://www.explainth.at/en/html/noscripts.shtml. It just is not valid HTML. Maybe that's not *that* bad for batch progress pages.Comment #42
treksler CreditAttribution: treksler commentedThis cookie is one of the dumbest things in Drupal.
We have over a thousand Drupal sites at www.domain.ca/site
The coockie domain is domain.ca. Modern browsers keep up to 50 cookies per domain. These useless has_js cookies routinely take up all 50 cookies available to the user.
It is a violation of RFC2109 section 6.3 to pollute the User Agent with needless cookies.
"Applications should use as few and as small cookies as possible, and
they should cope gracefully with the loss of a cookie."
I don't care how expensive it is to test JS availability without cookies, but if the browser is routinely discarding SESSION cookies and thereby logging out users, we have a major design flaw.
Since this cookie has the unintended side effect of randomly logging out users that manage more than a dozen multisites on the same domain, I think it should be marked critical and the cookie eliminated from even Drupal 5 where it is shoehorned in via the jstools module...
Comment #43
treksler CreditAttribution: treksler commentedComment #44
aspilicious CreditAttribution: aspilicious commentedDoes this "break" any sites else this isn't critical...
Comment #45
ksenzeePlease read http://drupal.org/node/45111. This is major at most. And since there is not even a proposed patch, there's no way we'll fix it in time for Drupal 7.
Comment #46
treksler CreditAttribution: treksler commentedYes, this prevents creating content.
I am assuming data loss is fairly critical.
Sign into about 10 sites on the same domain .. do work in each site.. Poof, some sessions die in the middle of editing a form or creating content. All because Drupal keeps spewing has_js cookies into the browser.
With this cookie, Drupal does not scale.
The patch is to delete the line that sets the cookie. I am sure one line can be deleted in time for Drupal 7.
Sure Batch API will not use JS this way but at least nobody loses their work. The JS enabled Batch API can wait for Drupal 8.
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commentedAgreed that bugs should be against Drupal 7 except under extraordinary circumstances.
But I don't understand how this becomes a serious bug - i.e., I don't understand what kind of setup results in being flooded with 'has_js' cookies. I have run lots of Drupal sites as subdirectories of localhost, for example, all the same domain. I only seem to have a single 'has_js' cookie as a result of that.
Comment #48
catch@David, I'm pretty sure you can set cookie domain for either foo.com or foo.com/bar (and foo.com/baz and etc.). So it may well depend on that.
edit: or thinking about it, if you had a mixture of those two configurations on one domain, you could definitely get into trouble.
Comment #49
treksler CreditAttribution: treksler commentedHi,
I did some more checking.
An older version of jstools.js for D5 set the cookie without the path, which resulted in the flood of many cookies per site.
document.cookie = 'has_js=1';
D6 and D7 and the latest jstools for D5 set one 'has_js' cookie per domain, because they explicitly set the path to '/'
document.cookie = 'has_js=1; path=/';
There is an issue (http://drupal.org/node/844282) that tries to "fix" this by using the base_path, and thus setting one cookie per subsite, which would result in a flood and logged out users.
Sorry for the panic,
I saw the flood on some D5 sites (3 cookies per site) and did not notice the cookies were set in a different way upstream :(
Comment #50
treksler CreditAttribution: treksler commentedThis cookie is still a bad idea though
1) All cookies are sent back to the server on every request. The less traffic, the better.
2) Is it possible to store the 'has_js' status in the session? i.e. use the cookie that's there already?
3) If something needs to know ahead of time whether the browser has JS, couldn't we just send it along via a GET variable. i.e. have JS append ?has_js=1 to the appropriate links? and have the batch API serve the appropriate page based on the presence of the variable
PS
I don't think you can have slashes in a cookie domain
Comment #51
catchIt's pretty unlikely that someone would get the 'has_js' cookie without also having a session, so that seems like a good idea. If it was a tiny patch, it might even get into 7 as a front end performance improvement - don't think much if any code accesses that directly, if not it's worth doing now and getting in early for D8 anyway.
Comment #52
marcvangendComment #53
amateescu CreditAttribution: amateescu commentedI'll try to take a stab at this in a few days. Subscribe for now.
Comment #54
nod_Sounds straightforward enough. Batch API still works and always output the same html page, haven't tested an update.
I went with the
<noscript>
approach. Turns out it's valid in HTML5 :)Comment #55
yched CreditAttribution: yched commentedIndeed, HTML5 allows noscript within the head section:
http://www.w3.org/TR/2009/WD-html5-20090423/semantics.html#the-noscript-...
http://www.roseindia.net/tutorial/html/html5/HTML5NoScriptTag.html
So yay for using noscript to only redirect when JS is off.
#54 looks fine, although I'm currently away from my coding env and can't do a full review right now,
Looking at the diff only, seems this function could go away, if it's just a wrapper around a _batch_progress_page() call ?
I'd move this comment above the block defining the whole $element, and phrase it something like : "Redirect though a 'Refresh' meta tag if javascript if disabled."
The comment should be more specific - that's something added by progress.js that we don't want in our case ? Then we should state why we don't need it and refer to the corresponding PHP code.
11 days to next Drupal core point release.
Comment #56
yched CreditAttribution: yched commentedLeaving at 'needs review'
Comment #57
nod_reroll
I'm not really good at comments :)
Also are we doing switch fall-through at all? I've introduced one here.
Comment #58
yched CreditAttribution: yched commentedcase fall-through: yes - when it's a missing 'break' statement we usually denote it in comments so that a good soul does not add it back later on, but when it's just chaining two 'case' statements it's ok as is.
"Non-Javascript base progress page" is not accurate anymore. Rather something like :
"Display the full progress page on startup and on each non-Javascript iteration."
11 days to next Drupal core point release.
Comment #59
nod_Here it is, thanks :)
Comment #61
nod_testbot failing, that was just a comment change.
Comment #62
yched CreditAttribution: yched commented#59: core-js-remove-has_js-cookie-229825-59.patch queued for re-testing.
Comment #63
Dave ReidAdding the required manual testing tag since this touches JS and the installer.
Comment #64
catchAdding WPO tag, this is upstream bandwidth every HTTP request including css/js/image files.
Comment #65
kosilko CreditAttribution: kosilko commented#59: core-js-remove-has_js-cookie-229825-59.patch queued for re-testing.
Comment #67
nod_That is wierd, patch apply cleanly for me.
Manually tested the install. Works with JS disabled, the cookie doesn't matter anymore since no back-end code rely on it.
Comment #68
nod_#59: core-js-remove-has_js-cookie-229825-59.patch queued for re-testing.
Comment #70
RobLoachYeah, we really don't need a cookie for has_js.
Comment #71
RobLoachComment #73
aspilicious CreditAttribution: aspilicious commented#71: 229825.patch queued for re-testing.
Comment #75
nod_We really need a testbot wizard looking at this issue.
Install goes well on local but fails here. Is testbot relying on the has_js cookie value?
Don't relaunch the tests if nothing changed about the testbots, it will just fail again.
Comment #76
Dave ReidStupid question: but have we manually tested a local install with JS disabled?
Comment #77
nod_meah i really thought I tried it before. Install fails with js not enabled. Sorry about that.
Comment #78
nod_The "only" thing left for this patch it to unbreak install.
After applying this patch Install fails. I am unsure what is the issue, is it the missing "has_js" cookie or is it the code changes in the batch callback? any help is appreciated.
Moving to base since the JS part of the issue is solved.
Comment #79
sun1) Stale 'do_nojs'.
2) The noscript-meta-refresh is a nice idea, but the refresh results in a HTTP GET instead of a POST.
3) Continuing on 2), how do you resolve the fact that a JSON response is returned to the user without JS?
Comment #81
Kiphaas7 CreditAttribution: Kiphaas7 commentedIt it goes out, it will probably need an announcement? Because I saw at least in admin_menu usage of
$_COOKIE['has_js']
Even though sun is the author of admin_menu and is actively involved in this topic, there might be more modules out there using the cookie to detect js in php.
Comment #82
nod_@sun 1),3), because you can't remove
$op = 'do_nojs'
2) Why is this an issue? never was a POST with js disabled.
Should work.
Comment #84
nod_#82: core-js-remove-has_js-cookie-229825-82.patch queued for re-testing.
Comment #85
Wim Leersnon-JavaScript
- s/though/through/.
- JavaScript.
- s/if disabled/is disabled/.
Better: Add the JavaScript code and settings for page loads where JavaScript *is* enabled.
Comments should wrap at 80 col. s/progressbar/progress bar/.
Comment #86
nod_Thanks :) fixed.
Comment #87
LoMo CreditAttribution: LoMo commentedTested and working. With JS disabled and enabled, the standard install process works smoothly. Nice! :-)
Comment #88
dasjotalked to nod_ and reviewed the patch. code looks clean + it works.
marking as RTBC
Comment #89
webchick5 files changed, 23 insertions, 73 deletions.
Wow, what a great patch! Removes a bunch of complicated, weird code and replaces it with extremely sensical code. :)
I also tested a non-JS install and it worked great! WOOHOO.
Committed and pushed to 8.x. Thanks!
Let's get a change notice for this.
Comment #90
nod_http://drupal.org/node/1739964
Comment #91
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #92
Kiphaas7 CreditAttribution: Kiphaas7 commentedReading some of the earlier comments again, should we support the edge cases where people have js turned off, AND are not willing (or unable) to support meta redirects? (lynx - unable according to some of the posts here and in #229905: Batch API assumes client's support of meta refresh ? and opera - can be disabled by the user?)
i.e. outputting text that the page _should_ redirect shortly, "if not, click this link... etc".
Temporarily setting back to needs work. Feel free to set back to fixed if this shouldn't be real issue. However, since we rely on it during install/upgrade, it might be worth thinking about it for a minute. The other topic might actually be a better place to discuss this, but this topic has actually had attention in the last 3 years compared to the other one. :)
Comment #93
nod_I'd prefer a follow-up since that'd be a feature request, it didn't work before and nothing is broken by this patch.
Comment #94
Wim LeersAgreed with #93.
Comment #96
lightsurge CreditAttribution: lightsurge commentedThanks for this @_nod!! This has been closed but as it's a bug, doesn't it get backported to 7.x? Apologies if I'm wrong, am I supposed to open a new ticket?
I manually patched 7.x with this and it seems to work fine, none of the stuff it's affecting seems to have substantially changed in 8.x... I could post a patch on here?
This would also close #608826: Install breaks without Javascript enabled if you have has_js cookie set as duplicate.
I think this really needs to get into 7.x, as with EU cookie law it's potentially against the law to install any cookies on a users computer unless they are strictly necessary, and when they say strictly necessary they mean so, or if you have a sufficient level of consent... because this just makes it possible to know when batch api could look pretty, it won't qualify, and because it's installed before you even have the chance to display any kind of warning, we can't even make the case of implied consent (i.e. implied by continuing to use the website despite the warning). I know this stuff seems fairly ridiculous and I'm boring myself just hearing myself fuss over the presence of a cookie like this one, but nor do I don't want to be the one trying to promote a product and get faced with the question 'Does this site install any illegal cookies?' and I have to answer 'Possibly, but it's only an itty-bitty, wafer thin possibly-illegal cookie'. Let's just get rid!
Comment #97
yched CreditAttribution: yched commentedThe fix that got in D8 relies on markup that is only valid in HTML 5 (
<noscript>
tag within the<head>
section - see #54 / #55). So I'm not sure this is applicable for D7.Comment #98
lightsurge CreditAttribution: lightsurge commentedw3schools says the
<noscript>
is valid in both HTML4.01 and HTML5 with no differences?http://www.w3schools.com/tags/tag_noscript.asp
Comment #99
lightsurge CreditAttribution: lightsurge commentedOh I see, within the head section
Comment #100
lightsurge CreditAttribution: lightsurge commentedThe tip in this seems like it might work:
http://stackoverflow.com/a/3613883
Comment #101
lightsurge CreditAttribution: lightsurge commentedOkay, I tried this in Chrome23, Firefox16 and IE9. It worked in Chrome and Firefox, not in IE9. IE9 ignored the
<meta>
tag with javascript disabled. I guessed IE9 was responding to some of the symbols in the javascript code within the<script>
tags (i.e., treating them like html when javascript was disabled), so I did this:And now it works in all these 3 browsers :-) And it's valid! I'll test in IE7+
I'm not sure I understand why the need for the final opening script tag though...
Edit: Duh, yes I do understand the need for it.
Comment #102
lightsurge CreditAttribution: lightsurge commentedCode in #100 and #101 only appeared to be working, but wasn't. In #101 As soon as first as the comment is inserted, all the code after it is commented out until the comment close (which because it needs commented out javascript to insert it, never comes).
Inspired by this though and inspired by all the reports saying this definitively can't be done, I think I've done it.
If javascript is working, it passes the meta tag into an unused variable. If javascript is enabled, the script is ignored, the meta tag gets seen, and the redirect triggers.
Tested working on IE7+, Chrome 23, Firefox 16 and Safari 5.1.7
Comment #103
lightsurge CreditAttribution: lightsurge commentedSo... here's a patch. Tested with fresh install of 7.x, and goodbye has_js cookie, but everything still works as before, with no invalid markup.
Edit: For info, with this patch, with javascript enabled, the markup becomes:
When disabled is:
The only gripe I can think of with the markup is that with javascript disabled, there's a script tag with just comment close characters inside (no open). Of course there's no js interpreter to read it, so it doesn't matter, but for the sake of completeness I could make it
/**/
which will still work for both cases.Comment #104
Kiphaas7 CreditAttribution: Kiphaas7 commentedMinifiers would possibly such a case where they trip.
Not to ruin your work, but isn't this a convoluted way of working around the fact that no script is not allowed in the header for certain doc types? But does not allowed equal does not work?
If it does work, I personally would not care much for not following the doc type to the letter. Especially because it is allowed in the new doctype.
Comment #105
lightsurge CreditAttribution: lightsurge commented@Kiphaas7 can you or anyone else explain or reference further how you think this won't validate? I'm honestly no guru on validation so would appreciate that, but having had a look I can't find any specific validation problems specifically in terms of
<head>
and only that this sort of code should be wrapped in a CDATA block in certain situations where I'm not sure it's relevant here?If necessary I can surely just add
//<![CDATA[
...//]]>
wrappings around the code to make it validate?And I'm not sure how you feel minification could be problem so would appreciate elaboration there too.
Comment #106
Kiphaas7 CreditAttribution: Kiphaas7 commentedRegarding non validation: I was talking about the solution committed to 8.x, not yours :). In other words' see 97, 98. 99
Regarding minifiers: you said your code introduces only an opening tag: as minifiers are basic interpreters, these could trip over that. Its a theoretical case because that particular piece of code probably never gets run through a minifier. But as this is core code and could serve as an example for contributed projects I'd say put the closing tag there as well.
Comment #107
lightsurge CreditAttribution: lightsurge commentedSo instead of doing
document.write('<script type="text/javascript">/*')
I should dodocument.write('<script type="text/javascript">/*</script>')
? That can't hurt, I'll amend, thanks for the suggestion.Comment #108
lightsurge CreditAttribution: lightsurge commentedAck, forgot document.write will blow up in xhtml and either break the site or it would try to do both forms of batch processing. No good then.
Comment #109
Kiphaas7 CreditAttribution: Kiphaas7 commentedWhich brings me to my original point: just because noscript is not allowed according to the spec in the header, I'm really wondering if it works just as well when using xhtml 1.0 or html4.01.
Comment #110
lightsurge CreditAttribution: lightsurge commentedFrom my perspective, breaking validation in a batch sequence is better than having cookies that potentially aren't legal without consent under some laws. I think @nod_'s patch should be backported as it is to 7.x, unless somebody offers some viable alternative, where given that if we have to offer
<meta>
refresh as a non-js fallback, I'm not sure exists.Comment #111
lightsurge CreditAttribution: lightsurge commentedJust to further note that the method in #103 wouldn't break Drupal batch processing in Drupal core OOTB, which might have been the impression I gave, as far as I tested it'd work quite possibly in all browsers and quite possibly in all DOCTYPES.
But if you installed the contrib module http://drupal.org/project/xhtml it would break batch processing.
The document.write functionality is only disabled, as far as I can tell, when a document is transferred with the application/xhtml+xml content type, but Drupal serves it up as html.
Comment #112
lightsurge CreditAttribution: lightsurge commentedAdditionally, given both issues in that modules queue suggest core and contrib incompatibilities with serving up Drupal pages in application/xhtml+xml anyway, maybe that's not as big an issue as I thought.
Comment #113
osopolarMid 2015: $_COOKIE['has_js'] is still not dead on drupal 7 :(
Comment #116
yonailo CreditAttribution: yonailo commented+1 to be ported to D7 !!
Comment #119
AnybodyAny planned progress on this for Drupal 7?
Comment #120
pounardNaive attempt.
Comment #122
pounardI guess that tests need to be fixed somehow.
Comment #123
giupenni CreditAttribution: giupenni commented+1 to be ported to D7
Comment #124
legovaerI had another go on this issue based upon the latest changes in D8.
Comment #125
legovaerOk the tests pass now and also the cookie is no longer set when I open Drupal. I will try to re-test some of the issues that have been identified in the history of this issue and will report back on them.
Comment #126
legovaerFor those who were waiting for this patch: I can confirm that I'm able to install a clean Drupal7 with the patch in #124 in Safari, Chrome and Firefox while javascript is disabled.
In none of these browsers the has_js cookie is set.
It would be nice to have a final review of the patch so that we can get this in after 10 years :-)
Comment #127
giupenni CreditAttribution: giupenni commentedI've tried the patch #124 in a fresh installation and seems works
Comment #128
FrederikvhoI have tried this out in Firefox, Chrome and Firefox and it works.
Firefox:
Safari:
Chrome:
Comment #129
FrederikvhoComment #130
klausiThe latest patch removes _batch_progress_page_js(), but there are still references to it in the code base ("@see _batch_progress_page_js()" for example). Please also remove those references.
Comment #131
ApacheEx CreditAttribution: ApacheEx as a volunteer and at Drupal Ukraine Community commentedHere is updated patch. I've removed reference to
_batch_progress_page_js()
.Comment #132
FrederikvhoI tested the latest version of this patch and it works fine.
Comment #133
sleitner CreditAttribution: sleitner commentedWill this patch be included in the 7.68 release?
Comment #134
mot-K CreditAttribution: mot-K commentedPerhaps in 7.70?
Comment #136
jonathan.green CreditAttribution: jonathan.green commentedFirefox throws a warning in the javascript console about this cookie:
Lacking the secure attribute also flags automated security scanners. Would be nice to get it removed as per this patch.
Comment #137
dsnopekI've manually tested doing normal batch operations, with Javascript enabled and with it disabled. I've also manually tested using the Drupal installer, with Javascript enabled and with it disabled. I tested in latest Google Chrome. All of those cases worked fine for me!
Comment #138
ressa CreditAttribution: ressa at Ardea commentedThis would be a nice little improvement, and since both @Frederikvho and @dsnopek has RTBC'ed it, perhaps the patch can get included soon, maybe in Drupal 7.76?
Comment #139
mcdruidThis LGTM.
I manually tested running through the installer with and without JS enabled in a browser (current firefox), and everything worked.
I also did the same in lynx 2.9 ("classic non-graphical (text-mode) web browser"). I had to manually confirm that I wanted to follow the REFRESH / redirect a few times (which may be a default setting that can be changed?), but everything worked fine. Screenshot attached.
I agree it'd be great to ditch the has_js cookie before it starts causing problems with samesite attributes etc..
I also did a quick grep through the top ~400 D7 contrib modules and didn't find any relevant references to / usage of the cookie.
Comment #140
Mile23Just to +1 on the RTBC for #131.
Comment #141
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedIn general:
RTBC + 1, it's really fragile to depend on the server on something that is client-side set.
BUT:
Some custom code could depend on has_js (as it feels convenient), so we would probably need to make it opt-out and removed for new installations [that's the line in drupal.js].
Our core changes are fine thought, except for one thing:
- There is some snippet that says that _batch_do can now work also with GET requests (and somehow did not work with them before?).
So we need to ensure that JS-disabled before also used GET requests, so that we don't have a change in behavior here.
Tricky ...
Comment #142
ressa CreditAttribution: ressa at Ardea commentedIt's great to see that this is moving along. noyb ("none of your business") just announced that noyb aims to end "cookie banner terror" and issues more than 500 GDPR complaints.
While
has_js
is not the worst kind of cookie, it's still great to start with a clean sheet in terms of cookies after installing a fresh Drupal 7, and one less cookie to deal with in general.Comment #143
mcdruidKeeping the Pending Drupal 7 commit tag, as we definitely want to get this in ASAP.
However, back to NW based on required changes outlined in #141
I think I'd make the argument that the change (removal of the cookie) should be opt-out so that sites which require the cookie can change a variable to keep it, but it's switched off by default for everyone.
We can argue which way around that should work once we've implemented the "opt-in/out based on a variable" functionality for the cookie though.
Comment #144
mcdruidThis still needs the
...to be added.
Then we can debate which is the default (well we could debate that before it's implemented, but it's not being committed and released without the opt-in/out ;)
Comment #145
mcdruidHere's a patch which adds a variable that can re-enable the has_js cookie.
The interdiff for drupal.js looks a bit weird for some reason; the old document.cookie lines look a like they've been added back in and then removed. Not sure why - only the new version is in the patch.
This seems to work in manual testing, but it'd be good to add a basic test which confirms that the variable behaves as expected.
I'd vote to handle it this way i.e. remove the cookie by default but allow it to be switched back on by sites that need it.
I've also added a SameSite=Lax attribute to the has_js cookie if it's set; without this we're back to the console warning (in some circumstances). For a simple boolean flag like this I believe that should be okay, but I haven't thought it through in too much detail yet.
I've not addressed @Fabianx's concerns about GET requests (in #141). I'll need to look at that more carefully.
Setting to NR for tests, but there are still todo's as mentioned above.
Comment #147
mcdruidHmm.. because the has_js cookie is set within the JS itself we're going to have a hard time testing that specifically in D7.
This patch adds a really basic test which tries to ensure that the boolean value of the
set_has_js_cookie
variable is reflected inDrupal.settings
on the page. This is far from perfect, but it'd hopefully catch a regression at that level.It's also not ideal that we're having to change a numerical index in one of the other JS tests relating to Drupal.settings but I think this is just down to how the test was implemented rather than being a real problem with how we're adding to the settings.
There's still the concern over GET requests mentioned previously, but hopefully this pretty much covers the opt-in/out for the cookie itself.
Comment #148
mcdruid@Fabianx I'm struggling to see what you meant in #141
What's the snippet you're referring to there?
Comment #149
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC, looks great to me now.
The comment was to something else in a previous patch, which is not present here.
Comment #150
mcdruidNoting that setting the samesite attribute to Lax on the cookie if it's re-enabled seems right to me.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/Sam...
The standard was updated recently such that:
This value means that:
That sounds like the right option AFAICS.
Comment #152
mcdruidAmazing to commit this patch and close an issue that's over 13 years old - thank everyone!
Comment #153
izmeez CreditAttribution: izmeez commentedWhat about the comment in https://www.drupal.org/forum/support/module-development-and-code-questio...
Comment #154
izmeez CreditAttribution: izmeez commentedMaybe the last comment #153 is more suited to a previously fixed issue or needs to be added as a new issue?
Comment #155
mcdruidYeah, so nothing we've changed here affects Drupal's session cookies. We added the (configurable) SameSite attribute to session cookies in #3170525: Set samesite cookie attribute for PHP sessions.
If the
set_has_js_cookie
variable is used to re-enable thehas_js
cookie, it will now have theSameSite
attribute hard-coded toLax
.I think that should be appropriate in the vast majority of cases (we can't use None as that's now invalid if the cookie doesn't have the Secure attribute).
If a site needs the
has_js
cookie andSameSite=Lax
is not appropriate for some reason, that'd have to be implemented in custom code (it could be done in a theme's JS easily) or perhaps a contrib module. In that case, they'd leave the functionality disabled in core (the default). I'd be surprised if that requirement came up much though.We should add a note along the lines of the above to the CR (which needs to be created for the next release).
Comment #156
izmeez CreditAttribution: izmeez commentedThere is a previous change record for Drupal 8, https://www.drupal.org/node/1739964 that does not say much. Maybe it could be built on?
Comment #157
izmeez CreditAttribution: izmeez commentedFurthermore, in comment https://www.drupal.org/forum/support/module-development-and-code-questio... it says,
Comment #158
mcdruidYup, that's about session cookies though. I don't think it's very likely that it'd be a requirement for a site to send the
has_js
cookie cross-site.We also cannot hard code
SameSite=None
as that's not valid without Secure.It doesn't seem worth the extra complexity of making the SameSite value for the has_js cookie configurable like we did for session cookies.
As mentioned, if sites really need the cookie, and
SameSite=Lax
doesn't meet their requirements, they'll have to roll their own, which would be simple to do. If it turns out there's a significant demand for it (and I'd be surprised) a contrib module could be created.