Posted by Robin Monks on March 4, 2008 at 2:26pm
20 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | javascript |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
- 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
#1
#2
Ouch. 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.
#3
Wait, 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.
#4
The bug report as it is now leaves some detail to be desired.
More accurate description here:
#5
From 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.
#6
Adding 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.
#7
Didn't mean to change the title.
#8
Argh.
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.
#9
True, this is a 'by design'
#10
Why? There's a simple fix.
Do
<?phpsetcookie('has_js', '');
?>
document.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),
setcookiewill 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.
#11
I tried this again in lynx, with no cookies, and it still fails:
#12
I feel the need to strangle project.module ;)
Robin
#13
#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.
#14
Original bug is also global to all places where $_COOKIE['has_js'] is used.
#15
#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
#16
The lynx thing is maybe caused by lack of meta redirects, as in http://drupal.org/node/204374?
#17
Array ( )
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
#18
"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
#19
Googling for "lynx meta redirect" suggests that it does not support those tags.
#20
OK, 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
#21
I 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
#22
Right... 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.#23
Can we just make people wait while the backend stuff is done? I mean, it's only about 7 seconds anyways
Robin
#24
(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)
#25
1.) 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
#26
Arancaytar++
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
#27
grr
#28
This 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.
#29
Agreed. My original blocking bug is now expressed in http://drupal.org/node/229905
Thanks all!
#30
Er, 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?
#31
On 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.
#32
Excuse 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..
#33
Moving 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.
#34
Just 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.
#35
#36
I 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.
#37
This 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.
#38
The "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.
#39
@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.#40
Doh, 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.
#41
According 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.#42
This 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...
#43
#44
Does this "break" any sites else this isn't critical...
#45
Please 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.
#46
Yes, 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.
#47
Agreed 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.
#48
@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.
#49
Hi,
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 :(
#50
This 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
#51
It'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.
#52
#53
I'll try to take a stab at this in a few days. Subscribe for now.
#54
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 :)#55
Indeed, 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,
+++ b/core/includes/batch.incundefined@@ -101,46 +101,7 @@ function _batch_page() {
function _batch_start() {
... (deleted lines)
+ return _batch_progress_page();
+}
Looking at the diff only, seems this function could go away, if it's just a wrapper around a _batch_progress_page() call ?
+++ b/core/includes/batch.incundefined@@ -221,9 +182,23 @@ function _batch_progress_page_nojs() {
+ // Don't redirect if javascript is enabled. This is valid HTML5.
+ '#prefix' => '<noscript>',
+ '#suffix' => '</noscript>',
);
drupal_add_html_head($element, 'batch_progress_meta_refresh');
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."
+++ b/core/misc/batch.jsundefined@@ -9,6 +9,8 @@ Drupal.behaviors.batch = {
+ // Remove the no-js fallback.
+ holder.empty();
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.
#56
Leaving at 'needs review'
#57
reroll
I'm not really good at comments :)
Also are we doing switch fall-through at all? I've introduced one here.
#58
case 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.
+++ b/core/includes/batch.incundefined@@ -72,7 +72,9 @@ function _batch_page() {
switch ($op) {
case 'start':
- $output = _batch_start();
+ case 'do_nojs':
+ // Non-JavaScript-based progress page.
+ $output = _batch_progress_page();
break;
"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.
#59
Here it is, thanks :)
#60
The last submitted patch, core-js-remove-has_js-cookie-229825-59.patch, failed testing.
#61
testbot failing, that was just a comment change.
#62
#59: core-js-remove-has_js-cookie-229825-59.patch queued for re-testing.