Posted by Xano on March 5, 2009 at 9:34pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | install system |
| Category: | task |
| Priority: | normal |
| Assigned: | Xano |
| Status: | closed (fixed) |
| Issue tags: | clean URL, Cleanup, Novice, UBUserTesting2009, Usability |
Issue Summary
During installation users are notified of the availability of clean URLs. If clean URLs are avaiable, the user sees the following message:
Proposal:
- Shorten the text to Make Drupal emit clean URLs (i.e. without
?q=in the URL). - Don't show the message if clean URLs are available. If the radio button is enabled, users will know it's available already. If it's not, _then_ they will need an explanation.
Comments
#1
Here's a quick patch that does this.
#2
And here's the patch. It fixes both problem during installation as well as at /admin/settings/clean-urls.
#3
I ran all tests and it passed.
#4
#5
- '#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.
#6
I will take Keith Smiths notes into account. Also, after a discussion with Bojhan and reading http://www.drupalusability.org/node/120 I will remove the clean URL form element from the install form.
#7
I got everything worked out, but the hotel blocks the ports needed to connect to the CVS server, so I won't be able to post the patch until tomorrow morning.
The current situation looks like this (/admin/settings/clean-urls)
The installer doesn't ask the user to enable clean URLs anymore. They are disabled by default, unless the JavaScript successfully tests if clean URLs work.
Since this is such a small setting, I have come to think we might even relocate it to someplace else.
#8
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.
#9
Bojhan and I agreed that it doesn't really matter if clean URLs don't work and the user doesn't know about it, because their site will still function as good as it will without clean URLs.
The verb may indeed be a very good idea.
#10
And here's the new patch.
#11
The last submitted patch failed testing.
#12
Testing bot just can't install Drupal. Locally it works without a problem.
#13
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/nodeinstead ofexample.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."
#14
We should indeed use something like "for instance" or "like". I'm with you on use paths like
example.com/nodeinstead ofexample.com/?q=node.Can somebody confirm this patch works and all tests pass and it is the testing bot that is wrong.
#15
I run through the installer with JavaScript enabled and disabled and in both situations Drupal could succesfully be installed.
#16
The last submitted patch failed testing.
#17
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.
#18
Are you getting any JavaScript errors? Also, what browser are you using?
#19
The last submitted patch failed testing.
#20
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.
#21
Core is independent from contrib and shouldn't be partial by suggesting using any of them. Also, I would only use paths that are present at all times for this example.
#22
Both good points. It seems like adding verbiage about what core can't do is a bit of a slippery slope anyway.
#23
Requesting a re-test since I believe the fail was due to a bug in the install profile.
#24
The last submitted patch failed testing.
#25
Let's see about this one. The label has been changed according to #14.
#26
The last submitted patch failed testing.
#27
@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.
#28
So what does this patch do?
#29
You know what it does, you've seen it in DC :P
- It removes the message that tells people clean URLs are available for the simple reason that if this is the case, the form elements are enabled.
- It removes the entire clean URL setting from the installer. Clean URLs are disabled by default, unless the JS code confirms they are available. Exactly the same behaviour as we have now, except for the possibility to disable clean URLs during installation.
Also, we got screen shots :)
#30
This patch removes another message that would be displayed at admin/settings/clean-urls after testing if they would work with JS disabled. Also, I changed the description to Use URLs like
example.com/userinstead ofexample.com/?q=user.. Paths may be confused with Drupal's menu paths and because of #375397: Make Node module optional I decided to use a path from User.module instead.The testing bot says he cannot install head with this patch. I tried reproducing the problem by disabling JavaScript and using the default profile, but after three attempts I never found any error.
#31
Here's a screenshot of the settings page. The only other visible difference is the removal of the entire clean URL form element from the installer.
#32
The last submitted patch failed testing.
#33
#420584: Clean URL form on install page and on admin/settings shouldn't show success, should "just work" marked as a duplicate.
#34
Completely hiding this sounds great. Subscribing, not reviewed yet.
#35
.
#36
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.
#37
Fixz0red.
#38
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?
#39
<rockstar>This is for all you pretty ladies out there!</rockstar>
//Edit: Should you wonder about it, the font size of text within code tags is being dealt with in #421076: Increase font size of text between <code> tags.
//Edit2: When testing locally all tests pass.
#40
The last submitted patch failed testing.
#41
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.
#42
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.
#43
The last submitted patch failed testing.
#44
Marking needs review, can't be tested.
#45
#46
What's this for in Drupal.behaviors.cleanURLsSettingsCheck()?
+ $("#edit-clean-url").addClass('clean-url-processed');#47
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.
#48
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*!"
#49
The last submitted patch failed testing.
#50
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
#51
I can confirm this baby works :)
Working on the settings page patch as we speak.
#52
I wonder why we still have the checkbox in the installer -- if we can enable it, why don't we just enable it silently?
#53
We do. Have you used the patch in #42?
#54
The installer shouldn't have any clean url checkbox in it with the patch. And the patch changes it to be enabled silently. :)
#55
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.
#56
Yay!
@Dries: check out #423196: Clean up clean URLs settings page.
#57
Automatically closed -- issue fixed for 2 weeks with no activity.
#58
Even shorter : “Don’t include
?q=in URLs.”