Drupal.behaviors.cleanURLsSettingsCheck() in system.js redirects to clean urls settings page incorrectly.

Confirmed. This is very, very broken, didn't look at why yet.

Interesting... just set up a fresh copy, and it works fine.

Is it possible you have clean URLs disabled?

also http://d7.local/?q=admin/settings/regional-settings redirects to http://d7.local/admin/settings/clean-urls while user is 1st.

I've tried to delete all cookies and clicking http://d7.local/?q=admin/settings/regional-settings then i got the page but with 403 coz i wasn't 1st. After loggin in i got again clean-urls redirect.

I'm confirming this bug as well. It popped up today when I checked out the latest D7-HEAD to do some patch work.

Let me run some tests to pinpoint this bugger.

Ok. Pinpointed the problem.

Currently, it affects any page that includes the file, modules/system/system.js. There are only two in core:
admin/build/block/configure/system/powered-by, and
admin/settings/regional-settings.

It's caused by the code changes made in #423196: Clean up clean URLs settings page. You can see the diff between HEAD and version prior to this patch here.

Prior to this change, the js function, Drupal.behaviors.cleanURLsSettingsCheck(), would simply modify form elements found in the clean url settings page if the server environment supported clean urls (regardless of whether clean urls is actually enabled). If that page was not being viewed, no changes would be made because the elements would be absent.

In the new version, the check is still being done, regardless of whether clean urls is actually enabled. The difference is that the browser is redirected to the clean urls settings page if the test is successful, even if clean urls is enabled.

Here's a patch. Basically, it halts the check (and the redirection) if the user is not currently on the clean urls settings page.

Prior to the patch from #423196: Clean up clean URLs settings page, the check only affected the clean urls settings page with the CSS selector, #clean-url.

Tests as working, RTBC

I'm not sure I understand the flow completely. AFAICT the Ajax check should only be done if the “Run the clean URL test” button is displayed, and that only happens if the page is displayed using a ?q=... URL and the drupal_http_request() call in system_clean_url_settings() fails (because the server cannot reach itself for some reason) - right?

In that case, isn't it better to replace the lines in system.js with this?

    if ($('#edit-clean-url-test').size() == 0) {
      return;
    }

I thought it was the other way around. Ajax is used as long as js is enabled. If js is not enabled, the script degrades gracefully to the "Run the clean URL test" form.

Well, I don't think we disagree. Note that I did not talk about JavaScript being enabled/disabled but about Drupal being able to contact itself using drupal_http_request().

If JavaScript is enabled, the page with the "Run the clean URL test" button is automatically replaced with the page with the checkbox. My question is: Is the Ajax used anywhere else than on the page with the button?

This patch fixes the problem by checking that the test button is being displayed at the page. It also removes some cruft from when there was still a clear url form in the installer.

Patch works as advertised.

Note to others who are reviewing this patch: I had to "Clear cached data" via Performance before it was fixed. It also works fine on a fresh install.

@c960657: Thinking about it again, yes we're actually agreeing. I was thinking about it in a reversed way and got confused.

The patch also works for me and the testbot is happy.

I'm unsure what the removed selector "#edit-clean-url.install" is supposed to address. It doesn't seem to be rendered in the clean url settings page, so I don't know where it shows up.

Otherwise, it seems good to go.

That selector applies to stuff in the installer that is no longer there. It's the cruft I meant. It was left in by the patch that cleaned up the installer.

Setting to back to RTBC - testbot was broken.

The patch works and makes sense, so it should go in, especially since it fixes a nasty bug :) However, I'm not sure why the patch removes the "clean-url-processed" class that previously got added post-processing; isn't adding those kinds of classes supposed to be best practice?

Also, I don't think this really addresses the root of the bug, which is that ultra-specific code like this (that only is intended to run on one page in Drupal) really should not be in a generic file like system.js at all, but rather in its own file that only gets included when needed. This would remove the need for using the jQuery selector, and also make it impossible for a bug like this to occur again. However, since the same criticism applies equally well to all the other functions in system.js, there's certainly no reason to hold up this patch over it :) Instead, I've created a separate issue at #492720: Refactor system.js file and use Drupal form states and form ajax.

Let's answer David's question first.

This was “fixed” in the duplicate issue #496922: "Powered by Drupal" redirects to "Clean URLs", though the fix was wrong AFAICT.

New patch.

However, the entire logic & workflow seems to be broken currently, so this patch is most probably wrong. The clean URL check already happens on the server-side. Only when the server-side check fails, system.js is loaded and tries to ...

Reconsidered. Attached patch should fix it properly.

subscribe

Sorry, caught the other patch first. :(

If/when human-testing for this patch succeeds (it worked for me), let's commit this, and I'll follow up with another patch that reworks that entire mess.

See also #492720: Refactor system.js file and use Drupal form states and form ajax

This still needs to go in, but is not critical at all.

Cross-referencing #881376: "Run the clean URL test" UX is broken

#21: drupal.clean-urls.patch queued for re-testing.

Removed non-canonical tag. See #2426171: Multiple tags similar to 'API clean-up' for background.