- 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

CommentFileSizeAuthor
#147 229825-147.patch9.28 KBmcdruid
#147 interdiff-229825-145-147.txt1.54 KBmcdruid
#145 229825-145.patch7.65 KBmcdruid
#145 interdiff-229825-131-145.txt1.67 KBmcdruid
#139 Selection_142.png33.5 KBmcdruid
#54 core-js-remove-has_js-cookie-229825-54.patch5.16 KBnod_
#57 core-js-remove-has_js-cookie-229825-57.patch6.05 KBnod_
#59 core-js-remove-has_js-cookie-229825-59.patch6.1 KBnod_
#71 229825.patch6.1 KBRobLoach
#79 interdiff.txt744 bytessun
#79 drupal8.cookie-has-js.79.patch6.64 KBsun
#82 core-js-remove-has_js-cookie-229825-82.patch6.38 KBnod_
#86 core-js-remove-has_js-cookie-229825-86.patch6.44 KBnod_
#103 remove-has_js-cookie-d7-229825-102.patch6.42 KBlightsurge
#120 229825-120-d7-has_js-must-die.patch2.67 KBpounard
#124 229825-124-d7-has_js-mist-die.patch6.38 KBlegovaer
#128 Screen Shot 2018-02-13 at 14.06.48.png35.69 KBFrederikvho
#128 Screen Shot 2018-02-13 at 14.08.55.png134.18 KBFrederikvho
#128 Screen Shot 2018-02-13 at 14.10.52.png81.44 KBFrederikvho
#131 interdiff-229825-124-131.txt362 bytesApacheEx
#131 229825-131.patch6.34 KBApacheEx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

keith.smith’s picture

Title: Drupal Install will produce a CURRUPT install if JavaScript is disabled » Drupal Install will produce a CORRUPT install if JavaScript is disabled
cburschka’s picture

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.

cburschka’s picture

Status: Active » Postponed (maintainer needs more info)

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.

cburschka’s picture

Title: Drupal Install will produce a CORRUPT install if JavaScript is disabled » Installer does not install core modules when JS is disabled

The bug report as it is now leaves some detail to be desired.

More accurate description here:

  1. The base schema is installed.
  2. Core modules such as menu, but also required core like user are not installed. Their schemas are not created, and their code will not get included in bootstrap.
  3. When visiting index.php after such a faulty installation, the lack of the required user module will cause a fatal error on calling user_access
keith.smith’s picture

Title: Installer does not install core modules when JS is disabled » Drupal Install will produce a CORRUPT install if JavaScript is disabled

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.

cburschka’s picture

Title: Drupal Install will produce a CORRUPT install if JavaScript is disabled » Batch API fails at detecting Javascript with $_COOKIE['has_js'].
Component: install system » javascript

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.

keith.smith’s picture

Title: Batch API fails at detecting Javascript with $_COOKIE['has_js']. » Installer does not install core modules when JS is disabled
Component: javascript » install system

Didn't mean to change the title.

keith.smith’s picture

Title: Installer does not install core modules when JS is disabled » Batch API fails at detecting Javascript with $_COOKIE['has_js'].
Component: install system » javascript

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.

yched’s picture

Status: Postponed (maintainer needs more info) » Closed (works as designed)

True, this is a 'by design'

cburschka’s picture

Title: Batch API fails at detecting Javascript with $_COOKIE['has_js']. » $_COOKIE['has_js'] is not cleared after Javascript is disabled.
Status: Closed (works as designed) » Active

Why? There's a simple fix.

Do setcookie('has_js', ''); at the start of every Drupal page, then do 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), 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.

Robin Monks’s picture

Title: $_COOKIE['has_js'] is not cleared after Javascript is disabled. » Drupal Install will produce a CURRUPT install if JavaScript is disabled
Component: javascript » install system

I tried this again in lynx, with no cookies, and it still fails:

* user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY fit DESC LIMIT 0, 1' at line 1 query:
SELECT * FROM menu_router WHERE path IN () ORDER BY fit DESC LIMIT 0, 1 in /srv/w***t/drupal/includes/menu.inc on line 315.
* user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY fit DESC LIMIT 0, 1' at line 1 query:
SELECT * FROM menu_router WHERE path IN () ORDER BY fit DESC LIMIT 0, 1 in /srv/w***t/drupal/includes/menu.inc on line 315.

Robin Monks’s picture

Title: Drupal Install will produce a CURRUPT install if JavaScript is disabled » $_COOKIE['has_js'] is not cleared after Javascript is disabled.

I feel the need to strangle project.module ;)

Robin

cburschka’s picture

Component: javascript » install system

#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.

cburschka’s picture

Component: install system » javascript

Original bug is also global to all places where $_COOKIE['has_js'] is used.

Robin Monks’s picture

#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

keith.smith’s picture

Component: install system » javascript

The lynx thing is maybe caused by lack of meta redirects, as in http://drupal.org/node/204374?

Robin Monks’s picture

Component: javascript » install system

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

Robin Monks’s picture

"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

keith.smith’s picture

Googling for "lynx meta redirect" suggests that it does not support those tags.

Robin Monks’s picture

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

Robin Monks’s picture

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

cburschka’s picture

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.

Robin Monks’s picture

Can we just make people wait while the backend stuff is done? I mean, it's only about 7 seconds anyways

Robin

Robin Monks’s picture

(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)

cburschka’s picture

Component: install system » javascript

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

Robin Monks’s picture

Component: javascript » install system

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

Robin Monks’s picture

Component: install system » javascript

grr

jbrauer’s picture

Priority: Critical » Normal
Status: Active » Closed (works as designed)

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.

Robin Monks’s picture

Agreed. My original blocking bug is now expressed in http://drupal.org/node/229905

Thanks all!

cburschka’s picture

Status: Closed (works as designed) » Active

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?

cburschka’s picture

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.

lil-1’s picture

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..

jbrauer’s picture

Version: 6.x-dev » 7.x-dev

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.

Everett Zufelt’s picture

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.

jbrauer’s picture

Version: 7.x-dev » 8.x-dev
Anonymous’s picture

Title: $_COOKIE['has_js'] is not cleared after Javascript is disabled. » $_COOKIE['has_js'] is not cleared after Javascript is disabled

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.

Damien Tournoud’s picture

Title: $_COOKIE['has_js'] is not cleared after Javascript is disabled » $_COOKIE['has_js'] must die

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.

yched’s picture

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.

Damien Tournoud’s picture

@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.

yched’s picture

isn't the nojs version based on HTML meta redirect headers

Doh, you're right, I should have actually checked before posting :-p.

it seems that <noscript> is valid inside <head>

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.

yched’s picture

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.

treksler’s picture

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...

treksler’s picture

Version: 8.x-dev » 7.x-dev
Priority: Normal » Critical
aspilicious’s picture

Does this "break" any sites else this isn't critical...

ksenzee’s picture

Version: 7.x-dev » 8.x-dev
Priority: Critical » Major

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.

treksler’s picture

Version: 8.x-dev » 7.x-dev

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.

David_Rothstein’s picture

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.

catch’s picture

@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.

treksler’s picture

Priority: Major » Normal

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 :(

treksler’s picture

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

catch’s picture

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.

marcvangend’s picture

Version: 7.x-dev » 8.x-dev
amateescu’s picture

I'll try to take a stab at this in a few days. Subscribe for now.

nod_’s picture

Status: Active » Needs review
FileSize
5.16 KB

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 :)

yched’s picture

Status: Needs review » Needs work

I went with the <noscript> approach. Turns out it's valid in HTML5 :)

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.

yched’s picture

Leaving at 'needs review'

nod_’s picture

Status: Needs work » Needs review
FileSize
6.05 KB

reroll

I'm not really good at comments :)

Also are we doing switch fall-through at all? I've introduced one here.

yched’s picture

Status: Needs review » Needs work

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.

nod_’s picture

Here it is, thanks :)

The last submitted patch, core-js-remove-has_js-cookie-229825-59.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

testbot failing, that was just a comment change.

yched’s picture

Dave Reid’s picture

Issue tags: +Needs manual testing

Adding the required manual testing tag since this touches JS and the installer.

catch’s picture

Issue tags: +WPO

Adding WPO tag, this is upstream bandwidth every HTTP request including css/js/image files.

kosilko’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +WPO

The last submitted patch, core-js-remove-has_js-cookie-229825-59.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

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.

nod_’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +WPO

The last submitted patch, core-js-remove-has_js-cookie-229825-59.patch, failed testing.

RobLoach’s picture

Issue tags: +JavaScript

Yeah, we really don't need a cookie for has_js.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
6.1 KB

Status: Needs review » Needs work
Issue tags: -JavaScript, -Needs manual testing, -WPO

The last submitted patch, 229825.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#71: 229825.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +JavaScript, +Needs manual testing, +WPO

The last submitted patch, 229825.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

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.

Dave Reid’s picture

Stupid question: but have we manually tested a local install with JS disabled?

nod_’s picture

Status: Needs review » Needs work

meah i really thought I tried it before. Install fails with js not enabled. Sorry about that.

nod_’s picture

Component: javascript » base system

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.

sun’s picture

Status: Needs work » Needs review
FileSize
744 bytes
6.64 KB

1) 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?

Status: Needs review » Needs work

The last submitted patch, drupal8.cookie-has-js.79.patch, failed testing.

Kiphaas7’s picture

It 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.

nod_’s picture

Status: Needs work » Needs review
FileSize
6.38 KB

@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.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Needs manual testing, -WPO

The last submitted patch, core-js-remove-has_js-cookie-229825-82.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Needs manual testing, +WPO
Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/includes/batch.incundefined
@@ -74,7 +74,9 @@ function _batch_page() {
+      // non-javascript iteration.

non-JavaScript

+++ b/core/includes/batch.incundefined
@@ -218,6 +163,9 @@ function _batch_progress_page_nojs() {
+    // Redirect though a 'Refresh' meta tag if javascript if disabled.

- s/though/through/.
- JavaScript.
- s/if disabled/is disabled/.

+++ b/core/includes/batch.incundefined
@@ -226,6 +174,17 @@ function _batch_progress_page_nojs() {
+  // Add the required javascript.

Better: Add the JavaScript code and settings for page loads where JavaScript *is* enabled.

+++ b/core/misc/batch.jsundefined
@@ -9,6 +9,8 @@ Drupal.behaviors.batch = {
+      // Remove HTML from no-js progressbar. The js progressbar is created later on.

Comments should wrap at 80 col. s/progressbar/progress bar/.

nod_’s picture

Status: Needs work » Needs review
FileSize
6.44 KB

Thanks :) fixed.

LoMo’s picture

Tested and working. With JS disabled and enabled, the standard install process works smoothly. Nice! :-)

dasjo’s picture

Status: Needs review » Reviewed & tested by the community

talked to nod_ and reviewed the patch. code looks clean + it works.
marking as RTBC

webchick’s picture

Title: $_COOKIE['has_js'] must die » Needs change notice: $_COOKIE['has_js'] must die
Category: bug » task
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

5 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.

nod_’s picture

Title: Needs change notice: $_COOKIE['has_js'] must die » $_COOKIE['has_js'] must die
Status: Active » Fixed
Tor Arne Thune’s picture

Category: task » bug
Priority: Critical » Normal
Kiphaas7’s picture

Status: Fixed » Needs work

Reading 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. :)

nod_’s picture

Status: Needs work » Fixed

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.

Wim Leers’s picture

Agreed with #93.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

lightsurge’s picture

Title: $_COOKIE['has_js'] must die » backport "$_COOKIE['has_js'] must die" patch to 7.x
Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Active

Thanks 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!

yched’s picture

The 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.

lightsurge’s picture

w3schools says the <noscript> is valid in both HTML4.01 and HTML5 with no differences?

http://www.w3schools.com/tags/tag_noscript.asp

lightsurge’s picture

Oh I see, within the head section

lightsurge’s picture

The tip in this seems like it might work:

<script type="text/javascript">
    document.write("<!-- ");
</script>
<meta http-equiv="Refresh" content="2;URL=js.html" />
<script type="text/javascript">
    document.write(' --><script type="text/javascript">');
</script>

http://stackoverflow.com/a/3613883

lightsurge’s picture

Okay, 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:

<script type="text/javascript">
    document.write(unescape('%3C%21--%20'));
</script>
<meta http-equiv="Refresh" content="2;URL=redirect.html" />
<script type="text/javascript">
    document.write(unescape('%20--%3E%3Cscript%20type%3D%22text/javascript%22%3E'));
</script>

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.

lightsurge’s picture

Code 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.

<script type="text/javascript">
    document.write(unescape('%3Cscript%20type%3D%22text/javascript%22%3E%20var%20hideme%3D') + "'");</script><meta http-equiv="Refresh" content="2;URL=redirect.html" /><script type="text/javascript">';
</script>

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

lightsurge’s picture

Status: Active » Needs review
FileSize
6.42 KB

So... 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:

<script type="text/javascript">document.write(unescape('%3Cscript%20type%3D%22text/javascript%22%3E') + '/*');</script>
<script type="text/javascript">/*<meta http-equiv="Refresh" content="0; URL=/batch?id=9&amp;op=do_nojs" />
<script type="text/javascript">*/</script>

When disabled is:

<script type="text/javascript">document.write(unescape('%3Cscript%20type%3D%22text/javascript%22%3E') + '/*');</script><meta http-equiv="Refresh" content="0; URL=/batch?id=9&amp;op=do_nojs" />
<script type="text/javascript">*/</script>

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.

Kiphaas7’s picture

Minifiers 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.

lightsurge’s picture

@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.

Kiphaas7’s picture

Regarding 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.

lightsurge’s picture

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.

So instead of doing document.write('<script type="text/javascript">/*') I should do document.write('<script type="text/javascript">/*</script>')? That can't hurt, I'll amend, thanks for the suggestion.

lightsurge’s picture

Status: Needs review » Active

Ack, 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.

Kiphaas7’s picture

Which 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.

lightsurge’s picture

From 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.

lightsurge’s picture

Just 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.

lightsurge’s picture

Additionally, 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.

osopolar’s picture

Issue summary: View changes

Mid 2015: $_COOKIE['has_js'] is still not dead on drupal 7 :(

  • webchick committed f047851 on 8.3.x
    Issue #229825 by nod_, Rob Loach, sun, yched: Fixed ['has_js()'] must...

  • webchick committed f047851 on 8.3.x
    Issue #229825 by nod_, Rob Loach, sun, yched: Fixed ['has_js()'] must...
yonailo’s picture

+1 to be ported to D7 !!

  • webchick committed f047851 on 8.4.x
    Issue #229825 by nod_, Rob Loach, sun, yched: Fixed ['has_js()'] must...

  • webchick committed f047851 on 8.4.x
    Issue #229825 by nod_, Rob Loach, sun, yched: Fixed ['has_js()'] must...
Anybody’s picture

Any planned progress on this for Drupal 7?

pounard’s picture

Status: Active » Needs review
FileSize
2.67 KB

Naive attempt.

Status: Needs review » Needs work

The last submitted patch, 120: 229825-120-d7-has_js-must-die.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

pounard’s picture

I guess that tests need to be fixed somehow.

giupenni’s picture

+1 to be ported to D7

legovaer’s picture

Status: Needs work » Needs review
FileSize
6.38 KB

I had another go on this issue based upon the latest changes in D8.

legovaer’s picture

Ok 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.

legovaer’s picture

For 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 :-)

giupenni’s picture

I've tried the patch #124 in a fresh installation and seems works

Frederikvho’s picture

I have tried this out in Firefox, Chrome and Firefox and it works.

Firefox: Screenshot of Drupal 7 installation with javascript disabled in Firefox

Safari: Screenshot of Drupal 7 installation with javascript disabled in Safari

Chrome: Screenshot of Drupal 7 installation with javascript disabled in Chrome

Frederikvho’s picture

Status: Needs review » Reviewed & tested by the community
klausi’s picture

Status: Reviewed & tested by the community » Needs work

The 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.

Frederikvho’s picture

Status: Needs review » Reviewed & tested by the community

I tested the latest version of this patch and it works fine.

sleitner’s picture

Issue tags: -JavaScript +JavaScript

Will this patch be included in the 7.68 release?

mot-K’s picture

Perhaps in 7.70?

  • webchick committed f047851 on 9.1.x
    Issue #229825 by nod_, Rob Loach, sun, yched: Fixed ['has_js()'] must...
jonathan.green’s picture

Firefox throws a warning in the javascript console about this cookie:

Cookie “has_js” will be soon rejected because it has the “sameSite” attribute set to “none” or an invalid value, without the “secure” attribute. To know more about the “sameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite

Lacking the secure attribute also flags automated security scanners. Would be nice to get it removed as per this patch.

dsnopek’s picture

I'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!

ressa’s picture

This 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?

mcdruid’s picture

Issue tags: +Pending Drupal 7 commit
FileSize
33.5 KB

This 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.

installing D7 in lynx without the has_js cookie

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.

Mile23’s picture

Just to +1 on the RTBC for #131.

Fabianx’s picture

In 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 ...

ressa’s picture

It'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.

mcdruid’s picture

Status: Reviewed & tested by the community » Needs work

Keeping 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.

mcdruid’s picture

This still needs the

"opt-in/out based on a variable" functionality for the cookie

...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 ;)

mcdruid’s picture

Status: Needs work » Needs review
FileSize
1.67 KB
7.65 KB

Here'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.

Status: Needs review » Needs work

The last submitted patch, 145: 229825-145.patch, failed testing. View results

mcdruid’s picture

Hmm.. 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 in Drupal.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.

mcdruid’s picture

@Fabianx I'm struggling to see what you meant in #141

- There is some snippet that says that _batch_do can now work also with GET requests (and somehow did not work with them before?).

What's the snippet you're referring to there?

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, looks great to me now.

The comment was to something else in a previous patch, which is not present here.

mcdruid’s picture

Noting 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:

The cookie-sending behavior if SameSite is not specified is SameSite=Lax.

This value means that:

Cookies are not sent on normal cross-site subrequests (for example to load images or frames into a third party site), but are sent when a user is navigating to the origin site (i.e., when following a link).

That sounds like the right option AFAICS.

  • mcdruid committed b8685e9 on 7.x
    Issue #229825 by nod_, mcdruid, sun, ApacheEx, lightsurge, legovaer,...
mcdruid’s picture

Status: Reviewed & tested by the community » Fixed

Amazing to commit this patch and close an issue that's over 13 years old - thank everyone!

izmeez’s picture

What about the comment in https://www.drupal.org/forum/support/module-development-and-code-questio...

Nice but what about the 'none' option. It also needs to have the 'Secure' parameter when using 'none'. How do we that?

cookie_samesite: None; Secure

In settings.php changed samesite=none to:

if (strpos($_SERVER['HTTP_USER_AGENT'], 'Chrome') !== false
 || strpos($_SERVER['HTTP_USER_AGENT'], 'CriOS') !== false) {
  ini_set('session.cookie_path', '/; samesite=Lax;');
}
izmeez’s picture

Maybe the last comment #153 is more suited to a previously fixed issue or needs to be added as a new issue?

mcdruid’s picture

Issue tags: -Pending Drupal 7 commit +Needs change record

Yeah, 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 the has_js cookie, it will now have the SameSite attribute hard-coded to Lax.

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 and SameSite=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).

izmeez’s picture

There 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?

izmeez’s picture

Furthermore, in comment https://www.drupal.org/forum/support/module-development-and-code-questio... it says,

The problem is that you need "none" when getting redirected back to drupal from Paypal or some other off-site payment gateway, otherwise the session cookie is not sent with the request.

mcdruid’s picture

Yup, 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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.