Remove clean URL availability message (needs manual review)

Here's a quick patch that does this.

-    '#description' => t('This option makes Drupal emit "clean" URLs (i.e. without < code >?q=< /code > in the URL).'),
+    '#description' => t('Make Drupal emit clean URLs (i.e. without < code >?q=< /code > in the URL).'),

If the "This option" part is unnecessary -- and I certainly agree that is -- why not shorten it further by removing "Make Drupal"? That'd also help out folks who want to white-label core for some reason.

Emit clean URLs (i.e., without < code >?q=< /code > in the URL).

or, if you want to avoid "Emit" and shorten it up further, there's always something like:

Do not include <code>?q=< /code > in URLs.

Edit: I fiddled with the code tags.

Looks good but:

- I am not sure, but usually we use some kind of verb on (checkbox) labels such as "Enable Clean URLs" / "Use Clean URLs" or similar?

- What about installer flow when clean URLs are not supported? drupalusability.com provides a solution: "If not available, let them know their server doesn't support the feature currently, along with some compelling reasons to have a server that does" -- is it taken into account in a patch?

+1 on moving the whole setting elsewhere but let's tackle it in followup issue.

If we're going with 'I.e. < code>example.com/node< /code> instead of < code>example.com/?q=node< /code>.', then how about 'Use example.com/node instead of example.com/?q=node.')'?

That puts it more in a line with some of the other descriptions, and eliminates the "I.e." -- there's another issue somewhere to remove all "i.e." and "e.g." abbreviations from core. Of course, while I think this is pretty clear, you could possibly make the case that this would appear to only affect the /node url. If that's an issue, then something like "Use paths like example.com/node instead of example.com/q=node."

Drupal is successfully installed with this patch, but now I'm unable to enable clean URLs.

With the patch, I don't get clean URLs enabled during install (the server does support them) and when visiting admin/settings/clean-urls the checkbox is disabled.

Turning off Javascript, visiting admin/settings/clean-urls and running the manual clean url check works fine and allows me to enable clean URLs.

So far, this addresses the second issue found by the usability testing, but not the first which involved participants expecting pathauto functionality out of clean URLs. I'm not sure this would do the trick, but one approach would be to use an example url where pathauto would typically come into play. For example (piggybacking off of Xano's last comment):

Use paths like example.com/node/2543 instead of example.com/?q=node/2543

Seems like beyond that, more verbiage would be required, such as:

Use paths like example.com/node instead of example.com/?q=node. For more search engine friendly URLs, use contributed modules like Pathauto.

It doesn't look Pathauto is ported to 7 yet, which would be another issue with this, but mostly I wanted to bring that other usability issue into the discussion for this patch.

Both good points. It seems like adding verbiage about what core can't do is a bit of a slippery slope anyway.

@stompeers:

I like your first suggested replacement:

Use paths like example.com/node/2543 instead of example.com/?q=node/2543

Although I think it should be:

Use paths like example.com/node/2543 instead of example.com/?q=node/2543

Either way, I think that the ?q= is the focus, not the nid.

So what does this patch do?

#420584: Clean URL form on install page and on admin/settings shouldn't show success, should "just work" marked as a duplicate.

Completely hiding this sounds great. Subscribing, not reviewed yet.

With the current patch, if clean URLs are disabled and the user visits ?q=admin/settings/clean-urls, the checkbox is disabled and there is now way to 'run' the clean url test.

Don't have time to do a full review but this looks like a good cleanup.

Can we pretty please have a screenshot of admin/settings/clean-urls?

There's still a problem with the patch. If clean urls are disabled and you visit ?q=admin/settings/clean-urls, and then try to enable clean urls, the settings do not save. It's because of $form['clean_url']['#disabled'] = TRUE; I'm working on a revision to this page that will help fix everything, but maybe we should just focus on improving the installer part here and work on a separate patch to fix the settings form.

Attached patch changes just the installer form to remove the clean url setting. Also fixed an error that caused the clean url JavaScript check to run twice because this condition no longer was met in system.js:

@@ -13,7 +13,7 @@ Drupal.behaviors.cleanURLsSettingsCheck 
     // This behavior attaches by ID, so is only valid once on a page.
     // Also skip if we are on an install page, as Drupal.cleanURLsInstallCheck will handle
     // the processing.
     if ($("#clean-url.clean-url-processed, #clean-url.install").size()) {

When changed to if ($(".clean-url-processed, #edit-clean-url.install").size()) { I confirmed that the clean URL check only ran once and ran correctly.

Note: the testing bot will report this back as code needs work because the PFIR script assumes the install form has a clean url checkbox. Needs manual review.

Marking needs review, can't be tested.

Basically it says, "Hey, I've just processed the clean_url value, so don't do it again in case JavaScript gets executed again."

Note that it's in the original code, just renamed since the field is a hidden form field instead of the checkbox / message div.

Patch Review of #42

Tested the patch in #42, and Working As Intended! On the initial installer screen, with Apache's rewrite_module enabled, nothing shows up on the form in regards to Clean URLs if the server will support them by default. Cool!

However, if Apache's rewrite_module gets switched off before the Drupal installation, we see the exact same thing! I'm really not quite sure why I can't see that SEO-friendly URLs (clean URLs) will be enabled for me by default during installation, as Xano's pic in #7 demonstrated.
As a newbie, I'd feel more comfortable knowing that Drupal's supposed promise of super-google stature will be delivered to me right out of the gate. Bummer on this point.

Once the site is fully installed, I visited the admin/settings/clean-urls page:

And then I enabled Clean URLs:

Then I shut off Apache's rewrite_module and refreshed the page to see what happens. The radio button pair correctly becomes disabled by jquery, and the "Your system configuration does not currently support this feature." message is correctly displayed. Woot!

Simpletest Report
System tests -- 744 passes, 0 fails, and 0 exceptions. No new Simpletests were created for this patch.

Summary Of The Test
With the exception of Clean URL's not being at all mentioned on the installation page, which I feel is sort of a mis-service to the general public, this patch is W.A.I. and can be committed with no danger. [UPDATE] After some discussion in IRC, I recant my original position on having the single, enabled checkbox appearing on the install.php form whenever Clean URL's have been tested successfully. It'd be the same as us trying to convince someone that "We've also installed GD on your site in case you wanna make your pictures *rock*!"

We shouldn't tout features during install time, it's not the place nor the time to do that.
And not showing it was the actual intent of this patch :-)

That said, thank you very much for this detailed and graphical review!

back to rtbc

I wonder why we still have the checkbox in the installer -- if we can enable it, why don't we just enable it silently?

The installer shouldn't have any clean url checkbox in it with the patch. And the patch changes it to be enabled silently. :)

Great! I didn't try the patch until now -- I overlooked that behavior when looking at the code. I've committed the patch to CVS HEAD.

The settings page has two titles called "Clean URLs" -- that is probably something that could be cleaned up a little still. The approach taken in #48 might be better for that reason.

Thanks for the hard work.

Even shorter : “Don’t include ?q= in URLs.”