Clean up clean URLs settings page

+++1. Will post my version that I wrote last night shortly and join forces.

Here's the version I've been working on last night and today. Here is the workflow for users that do not have clean URLs enabled:

1. User visits ?q=admin/settings/clean-urls.

2a. Check if clean URLs are currently enabled.
2b. Check if the request URI does not contain '?q='.
2c. Check if the session variable 'clean_url' has been set.
2d. If 2a-2c were all FALSE, then make a drupal_http_request to the clean URL test page and check if it was successful.

3a. If any of 2a-2d were TRUE, then show the clean URL checkbox and also set the 'clean_url' session variable.
3b. If 2a-2d were all FALSE, then show a page with a button to run the clean URL test. This page also has a JavaScript test that performs a request to the clean URL test page. If that request was successful, then it redirects the user to the clean URL form in 3a.

The session variable is used so that if/when people disable clean URLs, we can skip all the tests and display the settings form. I tested this with a lot of different conditions and they all degraded nicely.

I've also attached screenshots to show the initial ?q=admin/settings/clean-urls with and without javascript, and the revised admin/settings/clean-urls page after the test.

Just a simple reminder for later reviews:

if they uncheck the checkbox and press save. Because then if they get the test form again, the JS tests and sends them back to the settings form.

I think we should just add:

function system_help(...
    case 'admin/settings/clean-urls':
      return '<p>' . t("Clean URLs allow you to use URLs like <em>example.com/user</em> instead of <em>example.com/?q=user</em>.") . '</p>';

I don't like that the instructions are in a drupal_set_message and not in a markup form element...\

Doing this:

+  $var = variable_get('clean_url', 0);
+  $q = strpos(request_uri(), '?q=') === FALSE;
+  $request = drupal_http_request($base_url . '/' . '/admin/settings/clean-urls/check');
+  $session = drupal_get_session('clean_url');

Will cause a drupal_http_request every time when the other three conditions are easy to check first.

I agree with davereid that this should just be markup, and not a DSM.

Furthermore, the phrase "the checkbox above" should be avoided I think, unless it's a description on the actual element. Even then, I think dropping it and saying something more like "You can enable clean-urls once you successfully run the clean-url test" or something of the like would be better.

Additionally, the text feels overly verbose, but perhaps that's not a fair criticism as I can't readily think of a way to re-phrase it to have less text.

+    );
+      $form = system_settings_form($form);

There are many, many coding-style flaws in this patch.

- * Form builder; Configure Clean URL settings.
+ * Form builder; clean URL settings.

Previous looks better to me, please revert.

+  // Check if clean URLs are available. If ?q= appears in the URL, don't trust
+  // $var. This is a fallback for sites that have been moved to a server without
+  // mod_rewrite, for instance.

"If ?q= appears in the URL, don't trust $var." - Comments should be grokable without reading the code. If I need to read code, I don't any comment at all.

+  $var = variable_get('clean_url', 0);
+  $q = strpos(request_uri(), '?q=') !== FALSE;

Also, $var is a very poor variable name. Likewise, $q.

+  $available = $q ? FALSE : ($var || $q || $session ? TRUE : FALSE);

This looks very scary.

+  if (!$available) {
...
+  if ($available) {
...
+  else {

Duplicate and thereby superfluous control structure.

+    // Unset submit handlers to prevent them from being executes if clean URLs
+    // were checked manually.

Typo in "executes". However, I also don't understand what this comment tries to explain me. Don't explain in a follow-up, fix it in the comment.

Instead of:

+      '#markup' => '<p>' . t('Clean URLs makes your URLs look like example.com/user instead of example.com/?q=user. Please click the "Run the clean URL test" button to test if they are available. If you are directed to a "Page not found (404)" error, you will need to change the configuration of your server. The <a href="@handbook">handbook page on Clean URLs</a> has additional troubleshooting information.', array('@handbook' => 'http://drupal.org/node/15365')) . '</p>',

How about:

+      '#markup' => '<p>' . t('The "Clean URLs" feature rewrites URLs so that example.com/?q=user appears as example.com/user. Use the <em>Run the clean URL test</em> button to determine if this feature is available on your site. If you receive a "Page not found (404)" error, you must change your server configuration to use clean URLs. The <a href="@handbook">handbook page on Clean URLs</a> provides additional troubleshooting information.', array('@handbook' => 'http://drupal.org/node/15365')) . '</p>',

I've still got a few more wacky brainstorming ideas coming for this page... will post tomorrow.

I'm still not too excited about having the line "Use URLs like example.com/user instead of example.com/?q=user." in different places on the two different forms when it could just easily be a system_help('admin/settings/clean-urls') message. One time it appears in the block of text, the other time, it's tiny and below the checkbox. We should put it in the same place for consistency.

The internals of the form API is looking much better Xano! Much cleaner and better than some of the things that were in the previous patches.

Psst...remove the installer clean URL Javascript changes that were removed already in core. :)

Clean URLs don't make your URLs look like one thing or another. A Clean URL is one state of the feature that does that.

Instead of saying "see the online handbook", it is more appropriate to provide a one sentence explanation and maybe some instructions.

The current text sounds a bit like: "If you want to know how this ends, come back after the break".

Think about the people working in offline mode. We should try to help people on the page rather than sending them to another page. Don't send people to drupal.org unless strictly necessary.

Reroll for HEAD (and removed trailing commas after } which causes IE to barf)

Also, IMHO the message "Use URLs like example.com/user..." can and should be improved. It got me confused. Here is a suggestion:

• "Use URLs like example.com/user instead of example.com/?q=user." should go to system_help() as suggested in #23, and become:

  When enabled, URLs will not need the "?q=" and so will become example.com/user instead of example.com/?q=user.

• The rest of the description, which is specific to the manual test when the internal check fails, should let the user know that the feature is currently disabled. So I suggest:

  This feature is currently disabled; It can be enabled once the clean URL test below succeeds. If you are redirected to a Page not found (404) error after running the test, see the online handbook.

I believe you are working on making the 404 of the manual test go away all together, that would be great!

Xano, in regard to dismissing the annoying 404, how about having the clean URL test #redirect to the same form, with an extra argument i.e. 'admin/.../manual-run' and check for that argument if it exists and $available == FALSE then tell the user the test failed.

I hope this is the right time and place to say it.

Do we have to use the phrase "Clean URLs"? This, to some people, implies that the current URL is "dirty". Leisa and Mark made this remark almost immediately upon seeing it in their installation video at the 4:20 mark.

http://www.youtube.com/watch?v=7uSsnls16q0

I think "clean URL" is a computer centric term much like "daemon", "WYSIWYG" and "defrag".

I have general thoughts.

It should convey:
-Shorter, simpler and less complicated. But also we don't want to imply inferior.
-Easier to read and type
-Better SEO (It may be out of scope, but still relevant. Maybe a link to a handbook page explaining pros/cons?)

1. Simple URLs
2. Simple URL Paths
3. Easy and Simple URL Paths
4. Shorter URL Paths
5. Three characters shorter URL Paths
6. Why does my site have /?q= in the address bar? Can I remove it? (I'm kidding, but people get this question occasionally.)

This should increase confidence for new users to Drupal.

2. (edited) I missed the other recent commit to clean up the install page for clean URLs. I removed my 2nd complaint.

Also, do we even need a button to perform the test. Can we auto-test upon load of this page, check silently, and report the results without showing a 404 white screen of death? Can we load the test into a hidden IFRAME or do some jQuery AHAH magic to check the result? Can we show a progress bar/throbber during the test? If we can, we can gracefully degrade to a manual test if the browser fails an IFRAME/javascript support check.

Works for me, although I didn't go through the effort of actually disabling mod_rewrite, so...

Manual Patch Testing for #30

I visited the clean URLs page (with Apache rewrite enabled) and I got the obligatory "Enable clean URLs" checkbox with a touch of descriptive text. Cool so far.

Then, I turned off Apache's rewrite module and restarted the server. When I refreshed the ?q=admin/settings/clean-urls page, I was greeted with the exact same screen. Drupal was not testing for the possibility of clean URLs and relaying the lack therof to the end user. If I had enabled this checkbox and submitted the form, I'd probably have been greeted with a drupal_set_message explanatory error. This behavior would be a step backwards in terms of usability, because you forced me to take one extra, unneeded step.

Simpletest Results
There are no changes to Simpletests in this patch. All existing Simpletests still pass.

Manual Patch Testing Summary
Fail. The patch does not properly test for the absence of Apache's rewrite module upon page load. Fix that, and it'll be RTBC.

I re-applied the #30 patch and get the same problem. When I visited the ?q=admin/settings/clean-urls page, I was not warned that Clean URLs are unsupported by my server. Drupal was not testing for the possibility of clean URLs and relaying the lack therof to the end user.

Here's what it's not doing, but should be. If Apache's rewrite module is not enabled, and the user visits the ?q=admin/settings/clean-urls page, they are asked whether they want to enable Clean URLs. There's no error checking whatsoever here, so I was able to enable the Clean URLs even though the server did not support them, which then left me on the /admin screen with all links unusable.

Just comment out

RewriteEngine on

or change it to "off" in your .htaccess file to quickly disable Apache's URL rewriting.

Sun, I have MAMP Pro, which has a handy little checkbox for turning the rewrite module on or off, plus a restart all services button. I'm pretty sure that rewrite is being disabled during my testing due to the results I'm seeing. Thanks for the tidbit tho, that info probably should be posted in our handbooks. At some point. :)

Xano, here's the deal. What I'm trying to convey to you is that I don't see a re-test being forced when visiting the /?q=admin/settings/clean-urls page. The patch review I did in #38 never touched on the admin/settings/clean-urls page, but only dealt with the distinct lack of clean URL support. I am assuming that anyone who arrives on this settings page will not have clean URLs support already on their server or else it would have been enabled for them during installation. (Due to that other patch that just landed.)

The User Stories (workflow) I perceive for arriving on this settings page is thus:
A. A user arrives on this settings page because they need to enable clean URLs and it wasn't enabled for them during installation.
B. A user arrives on this page because their server has lost support for clean URLs during a site migration. Localhost or dev server supported Clean URLs, production server does not currently support Apache Rewrite.
C. A user needs to switch off Clean URLs for some reason, i.e. SEO testing.

In User Stories one and two, which are the only ones I'm concerned with for this patch, the patched core is not responding correctly when the user visits the settings page at /?q=admin/settings/clean-urls, Apache Rewrite module is not present, and Drupal's Clean URL's have not yet been switched on.

I can make a screencast if my written explanation is not coming across? Sometimes people just don't get what I'm attempting to say.

I went back and did some more testing alongside Xano via IRC. Here's what I found.

This time, I left Mamp Pro's Rewrite module on, and instead disabled Apache Rewrite with a RewriteEngine off in the /htaccess file. Next, I made sure that $clean_url == 0 in the {variable} table. Also, I logged out and back in, since I'd missed the fact that there was now a drupal_set_session() that remembers if the Clean URL Test has been run. Woops! :)

So, upon visiting the /?q=admin/settings/clean-urls page, I saw the Run The Clean Urls Test button, and I clicked it. I was redirected to the homepage of the site, but my address bar now showed /admin/settings/clean-urls as the URL. Strange.

I then tried to navigate my way back to admin/settings/clean-urls to see if the checkbox was on, but lo and behold, I see the Run The Clean Urls Test button again! Noooo! So I checked the {variables} table to see what the $clean_url variable was set to, and it was still 0.

In summary, I can repeat this procedure over and over again, but never get Clean URLs enabled and functioning. I suspect that the drupal_get_session() is always returning TRUE once the first clean url check is performed, but since the first manual check redirects to the homepage, one can never get back to the settings page and enable the checkbox due to the presence of the session variable?

reviewing this one as part of review sprint - will post tomorrow.

I have applied the patch from #30 then and the look of the Clean URLs page changed as is should and the I was about to turn Clean URLs on and off without a problem.

I turned off Clean URLs and deleted the .htaccess file, after this I was still able to enable Clean URLs even through they didn't work.

My first longer review and code change overnight - so please bear with me ...

webchick made some necessary changes (in the old patch there was no javascript-side error handling in this case: mod_rewrite disabled and javascript on => display error message).

1. After webchick's core changes the ids / jquery queries in systems.js Drupal.behaviors.cleanURLsSettingsCheck don't apply because there is no html-element with the id "clean-url".
A patch, that alters the jquery-queries is attached. But there is a myriad ways of doing the jQuery manipulations and I am not confident which is the preferred - please review.
But all default cases (see below) work fine now on my machine ...

UPDATE: this was incorrect, sorry - 20090429
2. There is a corner case, that maybe needs to be resolved:
STATE: mod_rewrite enabled, js enabled
- the request in system.admin.inc:1744 drupal_http_request($base_url . '/admin/settings/clean-urls/check') fails - temporarily inaccessible or e.g. firewall prevents outgoing http requests to same domain
- the feedback will be "Your server has been successfully tested to support this feature."

But there is *no* way to access the "enabling" form - and when there is the help text should provide a link to that form or javascript should automatically redirect to that form (patch from #30 used a redirect).

3. I found establishing "what to test", really difficult. So I made a list of all cases that occur (i think) - maybe that's helpful in the finally resolving review:

CASE 1:
STATE:
- Disabled mod_rewrite, new login, Javascript off

WORKFLOW:
- Go to ?q=admin/settings/clean-urls
- "Run the clean URL Test"-Button

EXPECTED:
- On submit "Run the clean URL Test" we'll get a 404

CASE 2:
STATE:
- Disabled mod_rewrite, new login, Javascript on

WORKFLOW:
- Go to ?q=admin/settings/clean-urls

EXPECTED:
- See "Run the clean URL Test", form
- Automatic check via JS, telling me "this won't work".

CASE 3a:
STATE:
- Enabled mod_rewrite, new login, Javascript off

WORKFLOW:
- Go to ?q=admin/settings/clean-urls
- Enable "clean urls"

EXPECTED:
- See "Enable Clean Url" - Form
- Toggling clean url works with no additional redirects (no firefox "whining").

CASE 3b:
like 3a, with Javascript on.

The new patch patches HEAD directly.

HEAD has been changed in a way, that patch #30 can't be applied correctly anymore. I think webchick has taken parts of your patch #30 and incorporated into HEAD. I am not sure it would make sense to try to merge my changes into patch #30.

The JS patch makes a minor adjustment so that javascript-dom-manipulation in the use case 2 (mod_rewrite off, js on) works as it should ...

I am happy to merge my patch, but your patch #30 cannot be applied to HEAD and my impression is that webchick has already merged significant part of your patch into HEAD ... could you maybe check that and re-roll your patch so that it works with HEAD - i'll merge my patch into yours right away ... hope that's ok with you.

i am new to this, and am just trying to help, really ... since last night HEAD now includes *all* your changes for system.module from patch #30.

Your remaining patch for system.js IMHO does not do the right thing; it removes the "error"-handling ("// Check failed") in the javascript ... my new patch maybe fixes that - but it cannot be merged into #30 ...

but as i said, i am new to this and just trying to learn + help.

i have re-rolled #30 so that it passes against HEAD again ... i have made *NO* changes to the patch #30 as such (but for adding one line of comment in system.js)
The re-rolled patch works and does what it should basically ...

Two last comments:

- If you go to ?q=admin/settings/clean-urls (mod_rewrite off), then turn mod_rewrite on and then press "Run the clean URL test", that will succeed. But you will get the feedback "The configuration options have been saved"; this is misleading because you just ran "the test" and no configuration options have been saved.

- I would also maintain that the ajax-"on-error" event in system.js should be used to adjust the information displayed to the user. At that point it has been established that clean URLs won't work (almost 100% for sure) ... displaying an appropriate message would be a usability improvement, i think and could spare some people a 404-page ...

Unless you want me to provide patches for these two things, i'll just move along ...

Marking back to CNR for the test bot - and because the two issues mentioned in #57 looks like they need feedback rather than being show-stoppers.

This goes beyond the current patch scope, but why are we doing this via JS in the first place? Can't Drupal query its own site on the server?

Was this committed?
http://drupal.org/cvs?commit=218236 says so...

Looks like it.

The committed patch causes the bug reported in #479604: Drupal.behaviors.cleanURLsSettingsCheck() in system.js redirects to clean urls settings page incorrectly.. I'll leave this issue as fixed and we discuss this specific redirection bug in system.js at that issue.

I can't believe this got committed with all the findings to the contrary, but oh well. It's off to squash the next bug that's cropped up cause of this thread. Why do the little ones always get through?

/me needs a flyswatter with smaller holes in it. :)