2 years ago, we changed Drupal's .htaccess URL rewrite rules such that clean URLs do not internally map to a 'q' query string parameter (#284899-147: Drupal url problem with clean urls). Meanwhile, all modern web servers can route URLs like http://example.com/index.php/foo to index.php, since that is part of the CGI specification (http://www.ietf.org/rfc/rfc3875 section 3.2). Given that both clean and dirty URLs can work without Drupal's legacy "q=" pattern, it is a WTF that we still have so much code in Drupal that both sets and reads PHP's $_GET['q'] variable. Furthermore, this gets in the way of current work to refactor Drupal's request routing logic to leverage Symfony classes (e.g., #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel), both because Symfony's Request class initializes its path from the URL path (not $_GET['q']), and because we can't reroute the reading of $_GET['q'] to a method call on an appropriate $request object (whereas if all current Drupal code were to call a function like current_path() instead, that function could be progressively iterated to integrate with the appropriate object). Therefore, this issue aims to remove all legacy handling of 'q' as a special query string parameter, and uses current_path() and friends for all path related logic.

Doing this also provides the opportunity to remove 'clean_url' as a configuration variable. If the only difference between clean and dirty URLs is a leading 'index.php/', then we can treat that similarly to how we treat the global $base_url / $base_path variables: just like we autodetect if we're serving the 'foo' path at the URL http://example.com/subdirectory/foo rather than at http://example.com/foo, and adjust all URLs accordingly to be relative to 'subdirectory', we can do the same for 'index.php'. The logic is a little different and requires another global variable $script_path, rather than simply appending to $base_path, because URLs to files (e.g., css, js, images, etc.) need to be relative to $base_path while URLs to Drupal paths need to be relative to $base_path . $script_path, but other than that, the essentials of working with relative URLs are the same.

Prior summary that focused on the problems of a 'clean_url' configuration variable

Right now we enable clean URLs in the installer if we can, but then have a bizarre admin screen for checking and enabling the feature, as well as many, many weird hacks in core to support non-clean urls. Image derivative callbacks are one.

Instead of this, we could try to get a more robust clean url check in the installer itself (probably a dedicated .php file so it doesn't require the request to Drupal itself and a full bootstrap, which has also led to a tonne of hacks in the installer), then point people to how to get it enabled if it's not.

Clean URLs should work on 99.9% of web hosts, so it is really going to be people who are running their own servers who run into the issue, and if they're going to run a normal Drupal site with clean URL support they need to fix it anyway.

drush installs etc. are not going to be able to check, but if clean url support works then the site will anyway - and we won't have a crappy variable_get('clean-url') that can go out of sync any more.

Follow Ups

Upgrade issues

What about sites upgrading from D7 and earlier that have legacy content containing links to http://example.com/index.php?q=foo, as well as other sites / search engines / users with bookmarks who have those links? The suggestion in #29 is to require the site to implement a redirect solution, possibly with the help of a contrib module.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Title: Drop support for non-clean URLs » Drop support for dirty URLs
cweagans’s picture

+1000000. And subscribe.

joachim’s picture

Agreed. Using dirty URLs on a commercial project is completely unthinkable, and the only time I ran into trouble was on a server that was running Zeus web server -- and even then, I got it working.

Passing thought: could support for dirty URLs be taken care of in contrib for those that really need it?

podarok’s picture

Subscribe

possibly we need to provide docs for such servers like nginx... or included contrib module for them...
nginx can do clean urls nut completly non via .htaccess

Crell’s picture

Since we know we're switching our core HTTP handling to Symfony, I should note that Symfony does not use ?q=foo for its routing. It uses index.php/foo. That seems to work out of the box with no modifications, and then you can mod_rewrite it back to normal. But, I think it handles it automatically anyway without a toggle.

So we could drop "dirty URL" support as a matter of course when we drop in the new kernel. :-)

effulgentsia’s picture

Opened #1474018: Remove all traces of old-style dirty URLs, $_GET['q'], and 'clean_url' variable to do this in the WSCCI sandbox. The approach taken there is to still allow people to use /index.php/foo instead of /foo, and to make url() choose whether to output the index.php/ part based on whether the page was requested with it. And therefore, remove 'clean_url' as a config variable. Still a work in progress, but please comment there if you like.

catch’s picture

This sounds like a good plan to me, if we can remove the variable that'd be great.

I have a very old site that was migrated from an even older site that has one path alias (I thought it was more, but it looks like only one now) with index.php in the alias to match the old CMS. The same could be true for redirects. However I don't think there's a problem requiring clean urls to make that work, it just flashed through my mind briefly but let's forget it was ever mentioned.

Michelle’s picture

Clean URLs should work on 99.9% of web hosts, so it is really going to be people who are running their own servers who run into the issue, and if they're going to run a normal Drupal site with clean URL support they need to fix it anyway.

Not necessarily. In my case, I haven't managed to get clean URLs working on my local dev environment but they work fine on my (managed) VPS.

This is an edge case and I don't expect it to be a blocker but wanted to add $.02 from someone who is fairly clueless when it comes to server stuff since the folks on this issue are all people who are unlikely to ever not be able to get clean URLs going and may not realize this could cause some people problems.

cweagans’s picture

In my experience, it's been because somebody doesn't have the apache rewrite module enabled or because AllowOverride None is set for their docroot. However, that's only really a concern if you're actually setting up Apache. Most people use a package like Mamp or Xampp or similar bundles and don't really have to configure that kind of thing.

In other words, let's document the requirements (which I think is already done) and direct people to the various support channels if they still can't get it working.

catch’s picture

It sounds like if we go with effulgentsia's idea, then we'd be able to drop the configuration option, while still having at least some kind of fallback for when the server doesn't support it. That could be best of both. The main places it is annoying to support are the config option and the various switches in url(), overlay, image derivatives etc. which all have to account for it separately - if we can drop that requirement then there's no need to drop the support so seems well worth trying that first.

effulgentsia’s picture

Status: Active » Needs review
FileSize
83.54 KB

This patch:

  • Changes dirty URLs from /?q=foo to /index.php/foo as is being proposed in #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel to better integrate with Symfony's Request object setup and Routing.
  • Given above, $_GET['q'] has lost any remaining meaning it once had from long long ago, so this patch removes it, everywhere. It removes all special treatment of 'q' as a query parameter, period.
  • Removes 'clean_url' as a configuration option, and something that is ever tested for in the installer or otherwise.
  • url() now outputs links that contain index.php or not (i.e., dirty or clean) that are consistent with the request URI. In other words, if you access a Drupal page as http://example.com/index.php/foo, then links on that page include index.php, and if you access it as http://example.com/foo, then links on that page do not. A way to think of this is as 'index.php' as a pseudo-directory that's part of the base path for purposes of link creation (though it's not actually part of what base_path() returns).
  • Changes the JavaScript setting Drupal.settings.basePath to include 'index.php/' or not, as per above handing of links. This makes Overlay and other existing core JS places work as expected. We may want to introduce a separate JS setting variable, in order to distinguish base path for links to drupal pages vs. base path for other things (e.g., images) where 'index.php/' should be omitted even when necessary for the former.

This still has variable_get('clean_url') calls for file module, image module, and JS/CSS gz creation. I'm not sure how to handle those yet, but wanted to get this up for a bot check and human review in the meantime.

sun’s picture

Status: Needs review » Needs work

What a frickin' AWESOME patch is that?! :-)

+++ b/core/includes/bootstrap.inc
@@ -625,8 +621,6 @@ function drupal_settings_initialize() {
-    // $_SERVER['SCRIPT_NAME'] can, in contrast to $_SERVER['PHP_SELF'], not
-    // be modified by a visitor.
     if ($dir = rtrim(dirname($_SERVER['SCRIPT_NAME']), '\/')) {

This comment should be kept.

+++ b/core/includes/common.inc
@@ -2079,14 +2072,6 @@ function _format_date_callback(array $matches = NULL, $new_langcode = NULL) {
- *   - 'script': The script filename in Drupal's root directory to use when
- *     clean URLs are disabled, such as 'index.php'. Defaults to an empty
- *     string, as most modern web servers automatically find 'index.php'. If
- *     clean URLs are disabled, the value of $path is appended as query
- *     parameter 'q' to $options['script'] in the returned URL. When deploying
- *     Drupal on a web server that cannot be configured to automatically find
- *     index.php, then hook_url_outbound_alter() can be implemented to force
- *     this value to 'index.php'.

I don't think we want to remove this option. It allows code (and especially tests) to link to URLs that are delivered by other main application scripts than index.php.

We're merely changing its semantics - the "non-clean URLs" case just turns from update.php?q=foo into update.php/foo

+++ b/core/includes/common.inc
@@ -2726,6 +2688,13 @@ function base_path() {
 /**
+ * Returns 'index.php/' or '', depending on whether index.php is in the request URI.
+ */
+function front_controller_path() {
+  return (strpos($_SERVER['REQUEST_URI'], base_path() . 'index.php/') === 0 ? 'index.php/' : '');
+}

First impression on this function is: "odd." ;)

1) We don't separate between front and back.

2) It doesn't really return a "path."

3) The name is very long for a potentially often used helper that returns something short.

Let's think about alternative names. :)

+++ b/core/includes/path.inc
@@ -10,15 +10,11 @@
 function drupal_path_initialize() {
+  ($path = current_path()) || ($path = request_path()) || ($path = variable_get('site_frontpage', 'user'));
+  _current_path(drupal_get_normal_path($path));

That's "a bit" too compact ;)

Crell’s picture

Wow, OK, this is substantially more viable than I anticipated.

"Front controller" is a common architectural pattern used by nearly all modern applications, including Drupal. It's entirely descriptive. I don't mind it being a longer function name since few people should ever be typing it.

That said, how will this change again when we get the kernel in place, since the Request object does this sort of thing already? I forget if it's UrlMatcher or something else that generates URLs, which we'll probably want to do eventually as well.

kika’s picture

What about commented-out redirect in .htaccess to provide backward-compatibility for ?q= paths? Sorry if this is answered in this patch.

cweagans’s picture

@kika, the entire point of removing this is that it adds extra complexity and *nobody* uses dirty URLs. I don't think we need to provide an upgrade the path. The upgrade path is to fix your config.

webchick’s picture

Er. We do need to provide an upgrade path. I could very well have links in my node bodies to "?q=node/4"

cweagans’s picture

Right. You could. But who DOES? Really, who runs a Drupal website without clean URLs?

Again, that's the entire reason we're dropping it.

webchick’s picture

tstoeckler’s picture

Not sure if that really counts. See this one:
http://www.google.ca/#hl=en&sclient=psy-ab&q=site:drupal.org+inurl%3A%3F...

EDIT: Not implying that your examples don't count, I was trying to imply that maybe not all of the 9 pages actually have clean URLs enabled.

Michelle’s picture

Really, who runs a Drupal website without clean URLs?

People who don't know any better. People who tried once to get it working and hit a roadblock and gave up. People who don't care about SEO.

I think we need to be careful about making assumptions. Those who have the skills to write this patch are unlikely to run a site without clean URLs but that doesn't mean there aren't many out there who just don't for one reason or another. If we aren't offering an upgrade path, it needs to at least be very well documented.

cweagans’s picture

Okay, so say we decide to provide an upgrade path. How do we do it?

Our options:

1) Use an apache rewrite. This is not an option because the people that need this weren't able to get the ORIGINAL apache rewrite working in the first place, so why would they be able to get this working?

2) Edit their content for them. This feels dirty.

3) Leave in some kind of legacy support layer thing that watches for $_GET['q'] being set and route the request accordingly. If we do this, then we've gained nothing in this issue.

Any other options?

klonos’s picture

...move the related code to a contrib module and point people upgrading from D7 to D8 to it. After enough time will have passed and based on that project's stats we'll be able to know if our decision to drop support for dirty URLs was right or not ;)

Crell’s picture

Once we get a mechanism to register additional listeners into the kernel in place, it should be quite possible to write a contrib-based listener to check $GET['q'] and manipulate the request object as needed. So #23 is a viable option.

That said, as noted previously (I forget which of these forking issues it was noted in) there are legitimate uses for dirty URLs in development; it makes it so that you don't have to fight Git about .htaccess file changes when rolling patches or updating if you're not in /var/www. (Vhosts and subdirs both require editing .htaccess.)

catch’s picture

Title: Drop support for dirty URLs » Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support

This isn't really about actually dropping clean url support now (or at least not at the moment), so I'm re-titling the issue.

#22.1 is viable for sites with legacy ?q= incoming links that since switched on mod_rewrite and want them to redirect, so seems worth doing as well. I also think it's OK to say that if you were running your site without clean URLs in 6.x or 7.x, that you either have to figure out mod_rewrite or install a contrib project or live with some outdated links.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
85.07 KB

I still need to deal with File and Image module, but getting another bot check to see if the other tests now pass.

Status: Needs review » Needs work

The last submitted patch, drupal.q.26.patch, failed testing.

effulgentsia’s picture

FileSize
101.38 KB

Implements feedback from #12.

"Front controller" is a common architectural pattern used by nearly all modern applications, including Drupal. It's entirely descriptive. I don't mind it being a longer function name since few people should ever be typing it.

I removed the function and replaced with a global variable $script_path (I know, yuck, but we already have global variables for every other component of the URL, and I'm hoping this all gets cleaned up into a Request object when we get that far). I left the term "front controller" in some code comments, but not in any code.

We may want to introduce a separate JS setting variable, in order to distinguish base path for links to drupal pages vs. base path for other things

This patch reverts to Drupal.settings.basePath being the same as base_path(), and adds a Drupal.url() JS function for when URLs to Drupal pages are needed.

This also finishes the file, image, and css.gz parts. With any luck, bot will go green on this.

This patch also adds some new @todo comments that reviewers should evaluate.

effulgentsia’s picture

Status: Needs work » Needs review

move the related code to a contrib module and point people upgrading from D7 [sites with ?q= dirty URLs] to D8 to it

I agree. A simple redirect_q.module that implements hook_init(), checks for $_GET['q'], and does a drupal_goto() would be trivial. Or, this can get added, with configuration settings, to Redirect module.

effulgentsia’s picture

+++ b/core/includes/bootstrap.inc
@@ -646,6 +642,35 @@ function drupal_settings_initialize() {
+    if (isset($_SERVER['REQUEST_URI'])) {
+      $request_path = urldecode(strtok($_SERVER['REQUEST_URI'], '?'));
+      if (strpos($request_path . '/', $base_path . $script_path) !== 0) {
+        $script_path = '';
+      }
+    }

Calling attention to this. Maybe needs a @todo? request_uri() says that some servers won't provide a $_SERVER['REQUEST_URI']. In this case, #28 defaults to dirty URLs. Any thoughts on whether this is ok, and if not, what to do about it?

Status: Needs review » Needs work

The last submitted patch, drupal.q.28.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
102.43 KB
effulgentsia’s picture

Re #30, note that HEAD's request_path() function appears to not support clean URLs on servers that don't provide $_SERVER['REQUEST_URI'], so this patch doesn't make that any worse from what I can tell, unless I'm missing something.

Crell’s picture

So, one thing to consider with HttpFoundation and HttpKernel is that both support a base front controller that is not index.php. That's just the typical one, but Symfony full stack uses app.php and app_dev.php, which in turn allows you to have separately configured applications all using the same code. That would be extremely useful for, say, the installer or update.php, which could be only slightly modified kernels. If we can avoid hard coding index.php and simply support a "whatever the file was" logic like Symfony does, that's a lot of additional flexibility.

I don't know whether we can pull that off in this patch just yet, but it's a direction we should be moving.

effulgentsia’s picture

Most of the patch already does that, both in terms of determining the global $script_path, and in terms of having $options['script'] in url(). Where this patch retains hard-coding to index.php is here:

+++ b/core/includes/bootstrap.inc
@@ -646,6 +642,35 @@ function drupal_settings_initialize() {
+    // @todo Core scripts (e.g., 'core/install.php', 'core/update.php', etc.)
+    //   currently expect url() to link to Drupal's primary front controller
+    //   (index.php), rather than back to themselves.
+    //   - Do we want to refactor them to not expect this?
+    //   - Do we want to limit the below to core scripts only, allowing modules
+    //     to add custom front controllers and having url() be relative to them?
+    if ($script_path !== 'index.php/') {
+      // @todo Here we're assuming that when linking from another script back to
+      //   Drupal proper, we can use clean URLs. Provide documentation somewhere
+      //   (INSTALL.txt?) letting people using a web server where clean URLs do
+      //   not work know that they can manually add 'index.php/' in their
+      //   browser's address bar to view their site without clean URLs.
+      // @see http://drupal.org/node/1183208
+      $script_path = '';
+    }
effulgentsia’s picture

FileSize
104.91 KB

This deals with #30 by commenting that section, changing that code and HEAD code that use $_SERVER['REQUEST_URI'] directly to not do that (since we have a request_uri() function for just that reason), and adding some more @todo for existing problems in request_uri().

effulgentsia’s picture

FileSize
105.93 KB

This also fixes a pre-existing incorrect comment in web.config.

katbailey’s picture

FileSize
105.94 KB

This needed a reroll due to a recent change in overlay-parent.js.

Niklas Fiekas’s picture

Probably #1299424: Allow one module per directory and move system tests to core/modules/system and other file moving will prevent this from applying. Other changes also prevent auto-merging.
#38: drupal.q.38.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal.q.38.patch, failed testing.

nod_’s picture

Issue tags: +JavaScript

tagging.

Crell’s picture

Priority: Normal » Major

I talked with effulgentsia the other day and we concluded that this patch should go in before the kernel patch. That makes this a blocker, so bumping its priority.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
104.55 KB

Split, rebased, rerolled. Hope I lost nothing in the process.

Yay! Tests pass. Notes from manual testing:
Setup: Remove .htaccess, do a clean install.

  • Go to http://localhost/drupal-base-dir/, click the "Modules" admin link, getting redirected to http://localhost/drupal-base-dir/admin/modules that is not found.
    Partially expected - but maybe http://localhost/drupal-base-dir/ should redirect to http://localhost/drupal-base-dir/index.php/ if that doesn't add too much cruft or is at all detectable. Fine if not.
  • Go to http://localhost/drupal-base-dir/index.php/, click the "Modules" admin link, getting redirected to the correct page, but the overlay doesn't open.
effulgentsia’s picture

FileSize
104.55 KB

This just changes 3 places left over from prior patches that were incorrectly using baseScript to use scriptPath instead, which fixes the Overlay problem from #43.

Re #43.1, the key is we don't want to reintroduce a 'clean_url' variable, and we want to assume clean urls unless we have a good reason not to assume that. Some possibilities:

  • Make .htaccess redirect from http://example.com/ to http://example.com/index.php when mod_rewrite isn't enabled, but that wouldn't handle non-Apache, or Apache where .htaccess is removed or not configured to be read, or where mod_rewrite is enabled but doesn't work.
  • Document for people that if clean URLs don't work for them, they need to access their home page as http://example.com/index.php, not as http://example.com. Since you can't necessarily control what your visitors type in the address bar or what search engines index, this could require a contrib module to implement a redirect from the latter to the former. This is similar to, but different than #29, but could be added to a hypothetical redirect_q module or to Redirect module itself.
  • Rename index.php to something else (e.g., drupal.php), so that http://example.com returns a 404 on environments where URL rewriting doesn't work. I think the debate on whether or not to do that could get long, so I'd rather we defer that to a different issue.

What I'm working on next right now is reviewing the test changes in this patch. I think I was too aggressive in removing some tests, and need to add some back. I'll post another patch when I've done that. Otherwise, I think the code itself is ready, but still needs code reviews.

sun’s picture

um. http://example.com always works, because even without mod_rewrite, on PHP-enabled Apache servers, the built-in core mod_dir is (almost?) always configured to route into index.php through the DirectoryIndex directive, or the recently added FallbackResource directive (which has the potential to make mod_rewrite entirely obsolete in the future). Renaming index.php into drupal.php or anything else would break the front controller even in that fallback scenario.

sun’s picture

effulgentsia’s picture

FileSize
106.33 KB

From #38 to #43, robots.txt and web.config changes were accidentally removed. This brings them back. I reviewed the whole patch again, and everything looks right to me except common.test and locale.test. Those I need to study a bit more to make sure we're not removing valuable tests.

Re #45: Right. The concern raised in #43.1 is that if http://example.com is allowed to work on sites where clean URLs don't work, then with this patch, all the links on that page will be to clean URLs anyway, and those links won't work. So, on a non clean URL environment, better to somehow make http://example.com not work or redirect to http://example.com/index.php. I think a Drupal-based redirect would best be left to contrib, but I'm open to suggestions for .htaccess and web.config changes.

effulgentsia’s picture

FileSize
107.96 KB

This makes path.test and locale.test tests more robust in working whether you run tests with clean or dirty urls. I don't have these explicitly checking both clean and dirty separately, because they don't do that in HEAD, and I don't think it's necessary as long as common.test has sufficient coverage of url(). I still need to make sure it does, but I need to stop for the night and pick up again tomorrow. No need to hold up code reviews though on merely finishing up common.test. There's 100K of other changes here that need reviews :)

effulgentsia’s picture

FileSize
112.65 KB

And this restores a couple tests to common.test. So, I think this is ready, and am looking forward to someone saying otherwise, or RTBC'ing it.

Status: Needs review » Needs work

The last submitted patch, 1183208-drupal-q-49.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
112.65 KB
effulgentsia’s picture

FileSize
114.09 KB

Reroll to chase HEAD.

cosmicdreams’s picture

I found many instances of GET['q] in drupal.sh located in the core/scripts folder. Should we update those too?

tnightingale’s picture

While I'm not extremely familiar with the internals of bootstrap.inc, the patch doesn't appear to be doing anything radically different so looks good to me.
Aside from core/scripts/drupal.sh (mentioned above), it catches all instances of GET['q'].
Patched codebase installs on my local system without issue and a brief click around hasn't revealed any errors.
It doesn't care whether index.php is included in the url or not.

Niklas Fiekas’s picture

Should we have an update function that deletes the variable?

Crell’s picture

Niklas: Probably yes, we should.

Niklas Fiekas’s picture

Adding an update hook for that. I am not sure how to fix drupal.sh, so that's still open.

cweagans’s picture

I am not sure how to fix drupal.sh

'rm drupal.sh' would be a good start. Does anyone really use it anymore?

katbailey’s picture

I agree it's probably about time drupal.sh was shown the door given that the motivation for it arose in those dark dark pre-drush days. However, it certainly deserves its own issue and sure enough, there is one for it: #1540390: Deprecate the drupal.sh script

In the meantime, so as not to let this dumb script hold things up, I've added a fix for it to the patch.

aspilicious’s picture

I can't judge the code stuff in this patch, too much black magic happening with requests. I can verify that every instance of $_GET['s'] is removed.

RobLoach’s picture

Status: Needs review » Needs work

VERY VERY CLOSE!!!

+++ b/core/includes/bootstrap.incundefined
@@ -597,7 +593,7 @@ function drupal_valid_http_host($host) {
 function drupal_settings_initialize() {
-  global $base_url, $base_path, $base_root;
+  global $base_url, $base_path, $base_root, $script_path;
 
   // Export the following settings.php variables to the global namespace

I understand we're moving from one global ($_GET['q']) to another ($GLOBALS['script_path']). Are there plans to remove it entirely? I guess with the Session/Request stuff?

+++ b/core/includes/common.incundefined
@@ -2161,7 +2149,8 @@ function url($path = NULL, array $options = array()) {
     'alias' => FALSE,
-    'prefix' => ''
+    'prefix' => '',
+    'script' => $GLOBALS['script_path'],
   );

Could we use global $script_path at the top of the function rather than referencing $GLOBALS straight up?

+++ b/core/includes/common.incundefined
@@ -4105,15 +4075,18 @@ function drupal_add_js($data = NULL, $options = NULL) {
+      // Instead of running the hook_url_outbound_alter() again here, extract
+      // them from url().
+      $scriptPath = $GLOBALS['script_path'];
+      $pathPrefix = '';
+      url('', array('script' => &$scriptPath, 'prefix' => &$pathPrefix));
       $javascript = array(

global $script_path; here too? ;-)

+++ b/core/modules/image/image.moduleundefined
@@ -869,10 +869,10 @@ function image_style_url($style_name, $path) {
   // that it is included. Once the file exists it's fine to fall back to the
   // actual file path, this avoids bootstrapping PHP once the files are built.
-  if (!variable_get('clean_url') && file_uri_scheme($uri) == 'public' && !file_exists($uri)) {
+  if ($GLOBALS['script_path'] && file_uri_scheme($uri) == 'public' && !file_exists($uri)) {
     $directory_path = file_stream_wrapper_get_instance_by_uri($uri)->getDirectoryPath();

global $script_path; plz? :-)

+++ b/core/scripts/drupal.shundefined
@@ -119,16 +119,15 @@ while ($param = array_shift($_SERVER['argv'])) {
         elseif (isset($path['path'])) {
-          if (!isset($_GET['q'])) {
-            $_REQUEST['q'] = $_GET['q'] = $path['path'];
-          }
+          $_SERVER['SCRIPT_NAME'] = '/' . $cmd;
+          $_SERVER['REQUEST_URI'] = $path['path'];

Probably don't need the drupal.sh change now that drupal.sh is going away.

+++ b/core/modules/system/tests/path.testundefined
@@ -221,7 +221,7 @@ class UrlAlterFunctionalTest extends DrupalWebTestCase {
     // Test outbound altering.
     $result = url($original);
-    $base_path = base_path() . (variable_get('clean_url', '0') ? '' : '?q=');
+    $base_path = base_path() . $GLOBALS['script_path'];
     $result = substr($result, strlen($base_path));
     $this->assertIdentical($result, $final, t('Altered outbound URL %original, expected %final, and got %result.', array('%original' => $original, '%final' => $final, '%result' => $result)));

Here too plz? :-) ... I understand it's possible to reference $GLOBALS directly, but having all global references at the top of the function cleans it up a bit.

-22 days to next Drupal core point release.

tim.plunkett’s picture

I don't know that we need to be messing with $GLOBALS['foo'] vs global $foo in this issue.

I happen to agree with catch in #1515196-4: Standardize retrieval of "global" variables to be at beginning of function:

I actually prefer the $GLOBALS syntax since it's more clear in context where the value is coming from - for example it makes it much more difficult to confuse global $user and $account and similar.

Still CNW for removing the drupal.sh hunk.

tstoeckler’s picture

Status: Needs work » Needs review

I don't see the point in breaking something intentionally in core. I fully support removing drupal.sh, but that's not for this issue. You could argue whether it is necessary to cater for drupal.sh if we know we are going to kill it, but since we already have a patch that does there is little point in removing that hunk.

Dave Reid’s picture

Agreed. We shouldn't be discussing global $foo vs $GLOBALS['foo'] although I will say like catch and timplunkett I prefer the latter as it's more self-documenting where the variable is coming from.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Sounds good with me! Let's off-load those discussions to #1515196: Standardize retrieval of "global" variables to be at beginning of function and #1540390: Deprecate the drupal.sh script. I'm RTBCing this one.

[EDIT] Made note of the follow ups in the issue summary.

Crell’s picture

Issue tags: +wscci-hitlist

Tagging

chx’s picture

Status: Reviewed & tested by the community » Needs review

Hold your horses. Did anyone test this on a FastCGI install? IIS? I do not see them even mentioned or considered.

cosmicdreams’s picture

I'll have an opporutnity to test this on FastCGI / IIS later today.

cosmicdreams’s picture

so far:

installation works fine
many pages work fine

What should I test?

Do you want me to tell my computer to run the complete test suite overnight?

effulgentsia’s picture

The test should be that:

Whether success or failure, please report the version of the web server you're testing on.

cosmicdreams’s picture

FileSize
54.87 KB

Here's what I can see

when I create a node and go to it. The path is localhost/index.php/node/1 . That worked
when I test content at localhost/node/1 it worked.

Recommend RTBC

I've attached my phpinfo

chx’s picture

Status: Needs review » Reviewed & tested by the community

OK then.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

I test content at localhost/node/1 it didn't work... Then I set this in the settings.php : $conf['clean_urls'] = 1 ... Then the path to my content worked at localhost/node/1

If that's what happened, then something is wrong. With the patch applied, there should be no code that does anything differently based on $conf['clean_url'].

cosmicdreams’s picture

Modified my comment. My perception was wrong. It works in both cases, without configuration change.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

cosmicdreams’s picture

Status: Reviewed & tested by the community » Needs review

Oh and as for version of IIS = 7.5 express

RobLoach’s picture

Status: Needs review » Needs work

Did a bit more testing on this and if you a2dismod rewrite, and visit the homepage without the index.php, links don't add the /index.php/ prefix. Using index.php off the bat works fine though.

Should we put some documentation in settings.php to make it default WITH the index.php, and instruct users that they can use clean_urls = TRUE to remove the index.php prefix?

effulgentsia’s picture

Status: Needs work » Needs review

See #43-#47. #79 is the expected behavior of this patch. IMO, this is acceptable, or at any rate, can be discussed further in a follow-up. But leaving to others to decide whether to agree and RTBC, or disagree and require resolution on that point before this goes in.

Niklas Fiekas’s picture

@Rob Loach: As of (point 2) http://drupal.org/node/1183208#comment-5883736 that is expected behavior. The suggestion was a contrib module that redirects / to /index.php or find nother solution that doesn't reintroduce a variable.
Everything said - crosspost.

RobLoach’s picture

If we have a follow up issue put together to figure out what to do with that, I'd recommend RTBC.

effulgentsia’s picture

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

FYI: I updated the issue summary. If anyone wants to improve upon it, go for it.

catch’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -648,6 +644,43 @@ function drupal_settings_initialize() {
+      // @todo Core scripts (e.g., 'core/install.php', 'core/update.php', etc.)
+      //   currently expect url() to link to Drupal's primary front controller
+      //   (index.php), rather than back to themselves.
+      //   - Do we want to refactor them to not expect this?
+      //   - Do we want to limit the below to core scripts only, allowing
+      //     modules to add custom front controllers and having url() be
+      //     relative to them?
+      if ($script_path !== 'index.php/') {
+        // @todo Here we're assuming that when linking from another script back
+        //   to Drupal proper, we can use clean URLs. Provide documentation
+        //   somewhere (INSTALL.txt?) letting people using a web server where
+        //   clean URLs do not work know that they can manually add 'index.php/'
+        //   in their browser's address bar to view their site without clean
+        //   URLs.
+        // @see http://drupal.org/node/1183208

Is there a follow-up issue for the first @todo? I'd rather all this explanation was in an issue than the code tbh.

+++ b/core/includes/bootstrap.incundefined
@@ -1488,13 +1521,22 @@ function drupal_validate_utf8($text) {
+ *
+ * @todo The above comment is no longer true. $_SERVER['REQUEST_URI'] is
+ *   available on IIS and probably other servers. Research the current state of

This is also better as a follow-up issue, we can just remove that comment for now no?

+++ b/core/includes/common.incundefined
@@ -4105,15 +4075,18 @@ function drupal_add_js($data = NULL, $options = NULL) {
+      // url() generates the script and prefix using hook_url_outbound_alter().
+      // Instead of running the hook_url_outbound_alter() again here, extract
+      // them from url().
+      $scriptPath = $GLOBALS['script_path'];
+      $pathPrefix = '';

This is a bit evil. While it's going to be refactored later, is it evilness that we could move to a helper function at all? I haven't read down to image styles but I'd assume something similar has to happen there too.

+++ b/core/includes/install.core.incundefined
@@ -1522,11 +1522,8 @@ function install_configure_form($form, &$form_state, &$install_state) {
-  // Build menu to allow clean URL check.

Are we sure there's no other side effects from removing the menu_rebuild() here?

+++ b/core/includes/install.core.incundefined
@@ -1827,12 +1823,6 @@ function _install_configure_form($form, &$form_state, &$install_state) {
-  $form['server_settings']['clean_url'] = array(
-    '#type' => 'hidden',
-    '#default_value' => 0,
-    '#attributes' => array('id' => 'edit-clean-url', 'class' => array('install')),
-  );

Happy happy happy.

+++ b/core/modules/image/image.moduleundefined
@@ -869,10 +869,10 @@ function image_style_url($style_name, $path) {
-  if (!variable_get('clean_url') && file_uri_scheme($uri) == 'public' && !file_exists($uri)) {

OK apparently the same level of tweaking isn't needed for image styles, that's encouraging then ;). However we're removing an API function and replacing it with a $GLOBALS check, could we factor that into a helper as well?

+++ b/core/modules/system/system.installundefined
@@ -1849,6 +1849,13 @@ function system_update_8007() {
 /**
+ * Removes the clean_url variable.
+ */
+function system_update_8008() {
+  variable_del('clean_url');
+}

grrrr #1348162: Add update_variable_*() but NIH.

None of these are really blockers, I couldn't find a lot less to complain about although I didn't do a line by line of the entire patch, so leaving RTBC a bit longer.

cosmicdreams’s picture

regarding catch's first request for a follow up issue: #1547184: Refactor Core scripts

effulgentsia’s picture

Thanks for #87. This addresses #86.1 and #86.2, and re-adds the robots.txt and web.config changes from #54 that got accidentally dropped from subsequent patches. Other than robots.txt, these are minor comment changes only, and the robots.txt changes are trivial, so leaving as RTBC.

This is a bit evil.

NIH. Opened #1547376: Allow url() or some other function to return individual components of an outbound URL and added a @todo linking to it. Adding parameters to url() might involve some debate, so not wanting to hold this issue up for it.

Are we sure there's no other side effects from removing the menu_rebuild() here?

I'm not. But it's what the code comment claims, and tests pass.

we're removing an API function and replacing it with a $GLOBALS check, could we factor that into a helper

Since you indicated that this isn't a blocker, I'd rather defer it, since it's just happening in 1 place and could get fixed with Symfony related changes. I'm hoping that before D8 is released, we remove all globals period.

Dries’s picture

I'm in support of this patch. It seems like for the most part this is a big improvement; except in a couple of cases where the url() API/use got a bit more complex/ugly. In any event, I'll let catch drive this home.

aspilicious’s picture

#88: 1183208-drupal-q-88.patch queued for re-testing.

aspilicious’s picture

retesting, just to be sure...

xjm’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -648,6 +644,32 @@ function drupal_settings_initialize() {
+      // @todo Temporary BC for install.php, update.php, and other scripts.
+      //   - http://drupal.org/node/1547184
+      //   - http://drupal.org/node/1546082

@@ -1488,13 +1510,15 @@ function drupal_validate_utf8($text) {
+ * @todo The above comment is incorrect: http://drupal.org/node/1547294.

It seems to me that these @todo should simply be removed, now that the issues are filed, though the ones for _current_path() are clearly relevant.

no_commit_credit’s picture

Meanwhile, trivial docs fixes. I didn't remove the @todo since I don't know that there will be consensus on removing them.
http://drupal.org/node/1354#functions
http://drupal.org/node/1354#hookimpl (scroll down regarding hook_update_N().

Status: Reviewed & tested by the community » Needs work

The last submitted patch, interdiff-1183208.patch, failed testing.

xjm’s picture

Status: Needs work » Reviewed & tested by the community

Misnamed the interdiff.

effulgentsia’s picture

It seems to me that these @todo should simply be removed, now that the issues are filed, [but] I didn't remove [them] since I don't know that there will be consensus on removing them.

Personally, I like having @todo in the code pointing out known WTFs and incorrect documentation/comments, so that someone working on some other problem and going through that code doesn't get tripped up, and can easily find the relevant d.o. issue if there is one. But if we have a standard to do otherwise, I won't object to removing them.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Talked to Larry and catch in e-mail. Catch doesn't have internet access for a couple days, and this patch is holding up some of the work Larry is doing. Therefore I just reviewed this patch again, and decided to commit it to 8.x. It is a very nice simplification that shouldn't negatively affect many people in today's world where modern web servers all support URL rewriting. Thanks to everyone who helped! :)

Niklas Fiekas’s picture

Status: Fixed » Reviewed & tested by the community

Looks like this commit doesn't appear. Did you push it?

Dries’s picture

I did push it. Sure you don't see it?

http://drupalcode.org/project/drupal.git/shortlog does not seem to update in real time ...

tim.plunkett’s picture

tim.plunkett’s picture

Status: Reviewed & tested by the community » Fixed
sun’s picture

Priority: Major » Critical
Status: Fixed » Active
Issue tags: +Needs change record

This needs a change notice.

xjm’s picture

Title: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support » Change notification for: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support
catch’s picture

I opened #1555598: Provide redirects for legacy ?q= URLs for the upgrade path issue.

webchick’s picture

This issue has been sitting for over 6 weeks without documentation, and we are well above critical thresholds atm. please get this polished off at your earliest convenience. Thanks.

Crell’s picture

The "new" approach for getting data about the current path is going to change a couple of times as we go forward with WSCCI. How should it be documented here? To current_path(), to Request->attributes->get('system_path'), or to "you're not supposed to care, that information isn't global anymore, deal"?

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

I'll take a stab at this.

effulgentsia’s picture

Title: Change notification for: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support » Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support
Assigned: effulgentsia » Unassigned
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record

I added 2 change records (they automatically show up at the bottom of the issue summary).

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

as it relates to #2251061: arg(0) returns path prefix with Language Path Negotiation enabled, looking for the issue for this @todo. It says temporary... but it was April 2012.

... ah @catch helped me find it in irc.

#2237001: Remove no longer needed _current_path() fallback for early bootstrap

@@ -2655,42 +2679,45 @@ function request_path() {
+ * @todo This is a temporary function pending refactoring Drupal to use
+ *   Symfony's Request object exclusively.
+ */
+function _current_path($path = NULL) {

+++ b/core/includes/bootstrap.inc
@@ -551,12 +551,8 @@ function drupal_environment_initialize() {
+  // @todo Refactor with the Symfony Request object.
+  _current_path(request_path());

+++ b/core/includes/menu.inc
@@ -2304,10 +2304,11 @@ function menu_get_active_menu_names() {
+  // @todo Refactor to use the Symfony Request object.
+  _current_path($path);

+++ b/core/modules/comment/comment.module
@@ -500,12 +500,10 @@ function comment_permalink($cid) {
+    // @todo Refactor to use Symfony's Request object.
+    _current_path('node/' . $node->nid);

+++ b/core/modules/language/language.negotiation.inc
@@ -209,9 +209,11 @@ function language_from_url($languages) {
+      // @todo Refactor with Symfony's Request object.
+      list($language, $path) = language_url_split_prefix(_current_path(), $languages);