Usability: clean URL test with jQuery

I think the current wording is appropriate - Clean URLs are either "Enabled" or "Disabled".

Tempted to won't fix...

This patch tweaks the wording in a page that should simply be redone from scratch. The usability is simply ridiculous now. To enable clean URLs, you must click a tiny link in a long-winded form item description, then change a radio button and then submit a form.

The old system of transparent detection and disabling was much better usability wise. If we can't have that back, enabling and disabling should at least be changed into an atomic operation, which will simply fail if clean URLs are not supported.

Why not use JavaScript to test for clean URL support, and degrade to the old link-based method if JS is not available?

I agree with Steven and webchick. I think the current wording is correct, but enabling clean URLs (which probably about 95% of Drupal sites will end up wanting to enable) is far too difficult. IMO it would be sweet to automatically enable Clean URLs upon install if the server configuration can handle it. I'm not too savvy on how to implement this, but if someone gives direction I'll take a stab. Thoughts?

And when I said webchick, I meant webernet. Sorry! :)

IMO, leaving the "ed" makes more sense and is more consistent with all of Drupal. On the edit content type page, you'll find lot's of EDs. :)

Default options:
PublishED
PromotED to front page

Default comment setting: (Radios)
DisablED

Attachments: (Radios)
DisablED
EnablED

I'll check out that JS test shortly.

Instead of doing the Ajax query after clicking the link, why not do it automatically in document.ready and react instantly: enable the form or show a helpful error message?

Well, to suggest is to volunteer. Here's a patch that performs the desired functionality and uses Gurpartep's ajax code.

It's kind of ugly so suggestions and/or patch updates would be great.

@RobRoy (#4) - you're right, it should be done automatically at install, but that's a separate feature from this.

Even without a JS method, it seems to me that the link to run the clean URL test should/must be made much more prominent.

Here's a revised version that includes the test link in #suffix rather than #description to make it more prominent.

works for me and seems like a nice improvement

Sorry, one more structural upgrade. This version creates a separate modules/system/system.js file with a reusable Drupal.cleanURLsCheck() function. This is a better design and will make it very easy to automatically enable clean URLs during installation.

I realized that I should use the Drupal.jsEnabled "global killswitch" and also make the url in Drupal.cleanURLsCheck an argument for abstractness.

Okay, one last tweak to the functionality to make the url handling more abstract. I have also implemented automatic clean urls during installation, so I know it works, but I'll post that separately.

Automatically enabling clean URLs during installation is at http://drupal.org/node/103059 (dependent upon this patch).

A couple minor points:

Why "system.js" rather than something more connected to its function like "clean_url_check.js"?

Also, aren't all the other js files all in /misc?

I emulated Steven's menu scout patch in which he creates a help.js and puts it in modules/help/. Color.module's color.js is also in the module's directory.

It's mostly an arbitrary judgment call, so for the filename+location I defer to Steven or Dries. I asked Steven in #drupal if he thought a system.js was okay and he said that putting jQuery code in a separate .js is generally better for debugging - interpret that as you like.

I would certainly agree that having it in a separate file is better- just a question of the naming, really. To me "system.js" implies something like the whole jQuery library, rather than this code.

patch applies cleanly, works as advertised under FireFox and Safari. Any IE users out there?

I checked with Steven on #drupal and he said that modules/system/system.js was fine with him.

As long as someone can verify that this works reliably with IE, it's RTBC and a great leap forward.

IMO- the current patch fixes a usability bug.

Okay I tested it on IE6 and it does indeed work.

Postponed until Drupal 6.

Syncing with HEAD.

This patch is extremely confusing.

What is check_type for (in the JS)? It doesn't say at all. Judging from the code, this is intended to serve as some sort of API that can be used anytime? This is not necessary. If JavaScript needs to know whether clean URLs are enabled, the value of the Drupal setting variable should be passed in through drupal_add_js(..., 'setting').

Only the clean URL settings page needs to perform the actual check. Besides, I'm pretty sure this line fails with clean URLs on, as it will not be relative to the base_path, but to the entire clean URL path:

+    url = location.pathname + "admin/settings/clean-urls";

I also don't get why you do the autoattach inline... system.js should only be added on the clean URL page, and it should use the same method as our other .js files.

Finally, you're mixing PHP and JS code styles and messing both up. PHP uses 'clean_url', JS uses 'cleanURL'. 'cleanurls' is not right. There are also weird spaces at the start of the t() string, which I assume exist to add space before the span. This is probably the worst way to do this and should be changed.

Please do not set patches to 'ready to be committed' unless it actually is, i.e. has been reviewed fully.

Thanks for the review Steven. Apologies for the unclear patch - attached is an updated version.

The point of check_type is to allow the check to be done on both the clean URLs settings page and the installation page. I have a companion patch at http://drupal.org/node/103059 to enable cleanURLs automatically during installation, which is dependent upon this patch. There are other ways of incorporating both options, so if you would prefer a different method that's fine (e.g. the jQuery hide()s could just silently fail if the IDs don't exist). I've added doxygen comments to describe the two arguments to the function.

This check is never done if cleanURLs is enabled - I don't see the point of doing so.

The autoattach is done inline because system.js cannot assume that it is on the clean URLs settings page. I give three reasons: 1) for general abstractness 2) so that clean URLs can be enabled upon installation 3) system.js will eventually be loaded on other pages - I already have another patch in the queue that needs to use it on a separate page for custom date-time formats, http://drupal.org/node/105039. I think it's best to do it inline from the beginning.

cleanurls and spaces in the translations are fixed.

Also, I apparently needed to update the parameters to $.ajax. I presume this was due to a jQuery upgrade since the last time I fully tested it (probably the 1.0.4 upgrade). Anyway it's fixed now.

I think this is a nice little improvement. I'd like to commit it, but I guess it could one more review from Steven. :)

• Code style is still way too messy and bloaty. The typeof checks can be simplified to checkType || 'settings'. t() strings still have spaces in front of them. I don't see any code style improvements.
• ID's such as this are cumbersome: #clean-url-extended-description. By altering the markup to be more structured, you can have shorter and more readable selectors.
• What happens if I install Drupal in a subdirectory here?
  + $.ajax({url: location.protocol + "//" + location.hostname + url, type: "GET", data: " ", complete: function(response) {
• + * The url used to test clean URLs.. This does not say anything useful. As far as I can tell, this has to be a root-relative URL with leading slash. Where is this documented? What URLs work here?
• The explanation for clean URLs should be a main feature on the page, not some description that has its most useful item tucked away at the end. The whole page needs to be redesigned from scratch to make sense.

This is not ready for core by far.

Updated patch.

1. Crap, I somehow didn't include the code style improvements. This should be fixed.

2. Any suggestions on the ID? I think it's easiest and fastest to just directly select by the id rather than base the selection on markup hierarchy.

3. It should work fine in a subdirectory as it defaults to include location.pathname. I updated the doxygen to clarify that base_path() should be used if the url is included as an argument.

4. Updated the doxygen - is this more helpful? Right now only one URL is needed: admin/settings/clean-urls (the default), but if it is checked during installation a separate url needs to be specified. If necessary I can remove the argument from this patch and specifically add it in http://drupal.org/node/103059

5. 99.9% of administrators will use the new automatic Ajax interface. For the .1% (or less) that have js disabled I have included an improvement to pull the check link out of the description and into a separate line that is more prominent. If any other improvements are needed to this functionality I think it should be a separate issue.

Sorry, there were a few extra spaces in the previous patch.

Synced.

The id's are still too long IMO. #clean-url should be a wrapper element, and you could select purely based on HTML tag name to target the right parts of text to hide.

Also, I think the doxygen should point out that this function is only to be used to do the actual check, not to just verify if Clean URLs are on.

Thanks, how about this? I made the doxygen a bit clearer, added a #clean-url wrapper div, and updated the selectors to use the html tags.

The code has come a long way it seems.

One question: it does support different 'types'. It defaults to 'settings' but it is not clear what other values can be passed it. Anything other than 'settings' it seems. The documentation is not clear about that.

Furthermore, the non-default version doesn't seem to get used anywhere. The documentation mentions that the installer could use this but as far as I can tell, it doesn't.

Lastly, can't:

+    if (response.status == 200) {
+      // Check was successful.
+      if (checkType == "settings") {
+        $("#clean-url input.form-radio").attr("disabled", "");
+        $("<span> "+ Drupal.settings.cleanURL.success +"</span>").insertBefore("#clean-url .description span");
+      }
+    }
+    else {
+      // Check failed.
+      if (checkType == "settings") {
+        $("<span>"+ Drupal.settings.cleanURL.failure +"</span>").insertBefore("#clean-url .description span");
+      }
+    }

be written as:

+    if (checkType == "settings") {
+      if (response.status == 200) {
+        $("#clean-url input.form-radio").attr("disabled", "");
+        $("<span> "+ Drupal.settings.cleanURL.success +"</span>").insertBefore("#clean-url .description span");
+      }
+    }

At which point the 'checkType' stuff is getting sorta ... weird. What is left to execute when checkType != "settings"?

Sorry, I forgot the else-clause -- you get the point hopefully.

Sorry, the confusion ultimately lies in me developing this patch simultaneously with http://drupal.org/node/103059 which uses this same function to enable clean URLS automatically upon install. That was my ultimate goal.

So, with only this issue there is no need for the checkType or url parameters, but with #103059 more abstractness is needed. If the Ajax is run on the settings page we want to mess with the html to display the result, whereas if done on install we just want to silently enable cleanURLs if it is supported. That is where the non-default version would be used - in that issue I pass in "install" as the checkType but it isn't explicitly tested for within the function because currently it does not need to execute extra code.

Rather than have a boolean variable for TRUE if "settings", FALSE if "installation", which would satisfy my two cases, I tried to make the function a bit easier to extend in case someone wants to use it in another situation that I haven't already thought of (maybe install profiles? I don't know). But this has clearly made it more confusing.

Does my explanation make sense? Perhaps the doxygen needs more description?

You are correct that the if/else hierarchy could be reversed. My thought in choosing the current style is that if the function were extended it would make more sense to group the code by success/failure rather than by checkType. Either way would work and I can definitely change it if you think the other hierarchy is more logical.

Maybe it's better to make 2 functions? When you compare what both execution paths (i.e. settinngs vs installation) have in common, it isn't a lot. The code might be easier to grok, if there was clear separation (two functions). Just an idea -- haven't actually tried implementing this.

You're right. With jQuery the javascript is so compact that it's simpler to make two separate functions. Here's a new version that changes the function to just include the necessary settings code.

I like this functionality A LOT. Every time we show that clean URLs page to people they sit there clicking it angrily for 30 seconds, saying "Why is it greyed out?!??!" The link to "run the clean url test" is not obvious at all. So I already like the fact that this patch moves that link out of the tiny, tiny description and into a more visible place when JS is disabled.

1. The delay between when the page is first loaded and the appearance of the "Your server has been successfully tested to support this feature." text is rather long (kind of freaked me out... I thought I was imaging things ;)) I don't know if there's any way around that, though...
2. When I renamed .htaccess to .htaccess-no, so Clean URLs won't work, the only message is "Your system does not currently support this feature." This is not helpful. Tell me what I can do to help solve this problem. Recommend that I check that my .htaccess file is in place, or recommend that I talk with my systems administrator to see that the server has mod_rewrite, or refer me to a handbook page, or something.

1. Updated version shows "Testing..." while it is waiting for the server response, and then outputs the result afterwards.
2. The failure text now includes a link the handbook page on Clean URLs.

Sooo nice. But I've got some nitpicks.

1) Everything kinda ran together:
"Testing clean URLs...your system configuration does not currently support this feature. The handbook page on Clean URLs has additional troubleshooting information."

2) The test failed like it was suppose to but as a previous Drupal user I had no feedback that it had failed. My thought process: "Oh, I guess the javascript thing isn't working, *click link* wait that's the handbook page... *back* *slowly read* oooooh."

Code looks good and functions solidly though so I haven't modified any of it.

All I've done in this patch is capitalize the success/failure sentences and wrap them in divs using the system "warning" or "ok" classes to provide color feedback. I then wrapped the testing section in a div and toggle it out of view when we get our AJAX response.

Again, thanks for working this out Chris, its very slick.

woops

*grumble* new file. this is the same patch with system.js included.

Looks great now! Thanks. Committed to CVS HEAD.

Oh, and keep it coming. :)

The JS doesn't honor work if your host has a port number bit (e.g., http://example.com:1234).

(Nice improvement btw! Cheers :-)

#51 enables the clean url test on sites running on a non standard port. Works with sites on standard ports as well.

Committed to CVS HEAD. Thanks.