#1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support going in means that ?q= URLs will no longer work.

This only affects two kinds of sites:

- sites that are not using clean URLs, to those I say meh.
- sites that have non-clean URLs indexed in search engines and/or links pointing to them. There are apparently lots of pages like that.

Potential options:

- Commented out .htaccess rules.
- contrib module that checks for the 1 query string and does a redirect.
- change notice (separate to the main one for the index.php change) pointing to the two.

Opening as critical task since we should have a way to not have 23 million+ pages turn into 404s overnight (or more accurately, over the 6-8 years it takes all the sites to upgrade to 8.x).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Niklas Fiekas’s picture

Priority: Normal » Critical
sun’s picture

Component: user.module » base system
Crell’s picture

My preference is for a commented out .htaccess rule. (Or rather two; one to redirect to the new clean URL, one to redirect tot he new dirty.) That's going to be the most performant, and presumably least work for us to maintain.

Niklas Fiekas’s picture

Yes, .htaccess rules would do. One point to consider: Mostly users who couldn't work with .htaccess use "dirty" URLs.

chx’s picture

Obviously .htaccess is out if they couldnt get rewrite to work in D8- why would it work in D8? Makes no sense. Now, a module which does 301 redirects, that makes a lot of sense.

catch’s picture

A lot of sites have legacy incoming links, even if they have mod_rewrite, like Drupal.org.

But yeah for actually supporting sites that can't get clean URLs to work then a module should work, but I think both are valid.

webchick’s picture

How are they going to enable a module if all their URLs are broken? :) I think either way we need a .htaccess fix.

Niklas Fiekas’s picture

They can simply use the new URLs. Like /index.php/admin/modules. Just legacy URLs with ?q= need redirects.

hosef’s picture

I am not sure if this is the correct place to put this.

When one runs a clean install of the latest version of D8 the last page of the installer has a link that says 'Visit your new site.' This link uses the old URL style which would cause ones site to appear broken even though it is in fact not broken at all.

Having the .htaccess rule you guys are planning would most likely fix the problem, however I still want to put this out there just in case something else needs to happen

Crell’s picture

If the installer still has a ?q= URL in it somewhere, then that's a straight up bug unto itself. Please file a new issue for that, preferably with patch if you can. :-)

catch’s picture

@hosef, if that link goes to index.php then that's a known issue, it's not related to this one which is for providing redirects for ?q= style URLs which were used prior to D7.

@webchick this won't break URLs within Drupal, it's only for redirecting URLs that are already linked to from elsewhere on the web or in bookmarks etc. So you don't get a broken site, but you might get a tonne of 404s.

aspilicious’s picture

Hmmm after reading this a module is a solution everyone can live with.
So should this be a contrib module or a module in core?

chx’s picture

That's a question??? http://drupal.org/node/65922

There is ALWAYS a path for updating your site with Drupal core.

aspilicious’s picture

Hmm if this can be contrib why not use a contrib module?
Can't http://drupal.org/project/redirect handle stuff like this or maybe this module http://drupal.org/project/globalredirect? There must be a module that can filter paths on ?q=

aspilicious’s picture

Global redirect should do the trick:

'If enabled, this option will redirect from non-clean to clean URL (if Clean URL\'s are enabled). This will stop, for example, node 1 existing on both example.com/node/1 AND example.com?q=node/1.')

This is a small module and should be easy to port to D8. It just need to get rewritten for the new kernel stuff.

chx’s picture

erm, I thought I was crystal clear: this is upgrade-path class material and must be core.

JamesK’s picture

I would like to push for this being a module and not a .htaccess rule so that it will work with "any" web stack.

catch’s picture

Issue tags: +Novice

OK let's do this:

- add legacy_redirect module with a short description and hook_help() entry explaining why you'd need it.
- implement hook_init() (we don't need this to fire for cached pages, a redirect means this won't get cached). This can check $_GET['q'] via the request object and do a drupal_goto() if it's set.
- the module should come with a test that requests a page with a q query string and asserts that it's redirected correctly.

I'm going to tag this novice, it's not a novice PHP task, but it should be doable for someone new to contributing to core.

bleen’s picture

Status: Active » Needs review
FileSize
2.98 KB

Giving this a try to get my feet wet in D8 modules....

This patch follows the guidlines laid out by catch in #18 except for the "via the request object" part. I'm still trying to figure out the symphony integration but this didn't make much sense to me. After looking through Symfony\Component\HttpFoundation\Request it seems that this class is meant to make a request and not provide info about the current request. Again, this is likely just my lack of understanding. If someone can point me in the right direction as to what catch meant here I'll take another swing.

Status: Needs review » Needs work

The last submitted patch, 1555598.patch, failed testing.

Crell’s picture

bleen: Actually it's the other way around. :-) The Request object is for the incoming request. The purpose of Symfony/Drupal/the code is to take in a Request object, do *magic*, and return a Response object.

Rather than using hook_init(), this should actually be using a request listener. That's basically the Symfony equivalent of a hook that fires when the request is first being processed. We have a number of them in core already that you can look at for examples. (Look in /core/lib/Drupal/Core/EventSubscribers. You'll want a class like that.) Remember you need to wire it up in a bundle class for your module.

Rather than issuing drupal_goto(), you should also create a RedirectResponse object (Symfony thing) that points to the path that we should redirect to, then set that as the response on the event you get passed.

If that doesn't entirely make sense to you, stop by #drupal-wscci in IRC. I or various other people can try to explain it better when it's not nearly 1 am. :-)

bleen’s picture

crell... I have this so far: http://pastebin.com/954ZSaix but I do not know if I am doing this correctly and I do not know how to "wire it up in a bundle class for your module". some advice (or better yet an example of something similar I can follow) would be great

aspilicious’s picture

Look at Drupal\Core\EventSubscriber\LegacyRequestSubscriber for an example in combination with Drupal\Core\CoreBundle.

To register the event we have to register our new subscriber to a Bundle. Each module can contain one Bundle. And it's structered like ModuleBundle.

So if my module is name foo, the bundle name is FooBundle. In your module you can register subscribers, this is going to change soon when the container gets compiled but you can see an example in: Drupal\bundle_test\BundleTestBundle. That file is located somewhere in the modules/system/tests directory.

So when you subscriber is registred you have to tell the system what function to call (in your case onKernelRequestLegacy) and when to call it. This is done by the "getSubscribedEvents" that you should add in your subscriber. This function builds an array of events. The keys of the array hold information when the event should be triggered in our case this is KernelEvents::REQUEST. More info on the symfony doc pages: http://api.symfony.com/2.0/Symfony/Component/HttpKernel/KernelEvents.html.

The values are arrays with the first item the function you want to get triggered and the second item a weight. In our case we want to call it very early so he weight should be low.

Here is an example from a core function

  /**
   * Registers the methods in this class that should be listeners.
   *
   * @return array
   *   An array of event listener definitions.
   */
  static function getSubscribedEvents() {
    $events[KernelEvents::REQUEST][] = array('onKernelRequestLegacy', 90);

    return $events;
  }
bleen’s picture

FileSize
5.54 KB

@aspilicious I re-read your suggestions in #23 a few times and I feel like this may simply be a bit out of my element here. I kindof get it, but I'm grasping. Attached is what I have so far but I have not gotten things to work.

I *think* I am correctly registering my EventSubscriber class and I *think* I am subscribing properly but I *know* am still doing something wrong.

marcingy’s picture

Status: Needs work » Needs review
Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/legacy_redirect/legacy_redirect.module
@@ -0,0 +1,32 @@
+/**
+ * Implements hook_init().
+ */
+function legacy_redirect_init() {
+  // Check the incoming request to see if it uses q=path/to/page. If it does,
+  // redirect to the index.php/path/to/page equivilent.
+  if(isset($_GET['q'])) {
+    drupal_goto('index.php/' . check_plain($_GET['q']));
+  }
+}

This entire hook is unnecessary.

+++ b/core/modules/legacy_redirect/lib/Drupal/legacy_redirect/EventSubscriber/LegacyRedirectSubscriber.php
@@ -0,0 +1,40 @@
+  public function onKernelLegacyRedirect(GetResponseEvent $event) {
+    $request = $event->getRequest();
+    $queryString = $request->getQueryString();
+
+    // Perform a redirect here. q=path/to/page should redirect to index.php/path/to/page
+  }

What you'd want to do in this method is determine if you need to redirect, and if so, create a RedirectResponse object and then call $event->setResponse($response); The rest will happen automatically.

+++ b/core/modules/legacy_redirect/lib/Drupal/legacy_redirect/EventSubscriber/LegacyRedirectSubscriber.php
@@ -0,0 +1,40 @@
+  static function getSubscribedEvents() {
+    $events[KernelEvents::REQUEST][] = array('onKernelLegacyRedirect', 10);
+
+    return $events;
+  }

The priority here should probably be higher. If we're going to redirect, it should be done as early as possible.

+++ b/core/modules/legacy_redirect/lib/Drupal/legacy_redirect/LegacyRedirectBundle
@@ -0,0 +1,28 @@
+    // @todo Remove when the 'kernel.event_subscriber' tag above is made to
+    //   work: http://drupal.org/node/1706064.
+    $container->get('dispatcher')->addSubscriber($container->get('LegacyRedirectSubscriber'));

The container compiler now works, so this is no longer necessary.

marcingy’s picture

Status: Needs work » Needs review
FileSize
5.29 KB

New version of patch basically doing all of the above, which I managed to work out before finding out Crell had done a review.

I am having an issue with the test not working locally, but manual testing of the module works.

marcingy’s picture

FileSize
5.29 KB

Increase the priority missed that part

Status: Needs review » Needs work

The last submitted patch, 1555598.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
5.3 KB

This should fix the tests.

Crell’s picture

This looks good to me visually. The url() function will get converted to use the generator once #1705488: Implement a generator for Drupal paths is done, but it's not in yet so don't wait up for it. If testbot likes this I'm fine with RTBC.

effulgentsia’s picture

FileSize
4.47 KB
6.17 KB

Thanks all for the work on this. What a nice demonstration of using an event listener over hook_init(): totally appropriate!

This cleans up some coding standards, adds a couple comments, adds one more test, and more significantly, removes the unnecessary url() call (see inline comment for why).

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

The changes in #32 make total sense to me,

Fabianx’s picture

Nice, and clean, +1 for RTBC.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/legacy_redirect/lib/Drupal/legacy_redirect/EventSubscriber/LegacyRedirectSubscriber.phpundefined
@@ -0,0 +1,57 @@
+    // Run earlier than all the listeners in
+    // Drupal\Core\EventSubscriber\PathSubscriber, because there is no need
+    // to decode the incoming path, resolve language, etc. if the real path
+    // information is in the query string.
+    $events[KernelEvents::REQUEST][] = array('onKernelLegacyRedirect', 500);
+    return $events;

I always thought the lower numbers were called earlier.
I can't find the documentation abou this. Anyone can?

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Whoops didn't mean to turn off rtbc

Niklas Fiekas’s picture

It looks like higher priority listeners are called earlier: http://api.drupal.org/api/drupal/core%21vendor%21symfony%21event-dispatc... uses krsort(), http://api.drupal.org/api/drupal/core%21vendor%21symfony%21event-dispatc... calls them in their natural order.

webchick’s picture

Assigned: Unassigned » Dries

Yes, the fact that Symfony and Drupal have completely opposite ideas about "weight" is going to lead to all kinds of "fun" in the future. :\

This is setting a bit of a precedent, so I feel like it would be good for Dries to sign off on this approach.

sun’s picture

Assigned: Dries » Unassigned

Yes, that is a problem, but completely irrelevant for this issue. Thus, I've filed the following issue for you:
#1825124: Provide an Drupal\EventSubscriber override that uses natural weights

Crell’s picture

There is no precedent being set here. All of the existing listeners are using Symfony priority order, not Drupal weight order. This is still RTBC.

webchick’s picture

Assigned: Unassigned » Dries

The precedent being set is the addition of a "Legacy X" module to core in order to deal with an upgrade path issue. Re-assigning.

catch’s picture

Yeah I'm not sure we need this in core, or at least there's three things to balance:

- whether it'll be maintained in contrib vs. core
- convenience for Drupal 7 upgraders (if they care about the legacy URLs, sites that have been using clean URLs for ever might not care at all, large sites might handle this in apache or varnish anyway).
- potential confusion for Drupal 8 users who have no idea what ?q= is and have to deal with a longer module list (the heathens!!)

If we don't add the module to core, then posting as a contrib project would be sufficient to mark this issue 'fixed' I think.

Nothing breaks if this is missing except some potential external (or internal and hard-coded somewhere) links.

Crell’s picture

Now that the code is solid I don't have a preference whether it's core or contrib. I defer to Dratch on that.

Dries’s picture

I think this can be maintained as a contributed module. No need to have it in core in my opinion.

webchick’s picture

Assigned: Dries » Unassigned

Great! Who wants to create the project? :)

cweagans’s picture

Assigned: Unassigned » cweagans

Yoink!

cweagans’s picture

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

Title: Provide redirects for legacy ?q= URLs » Change notice: Provide redirects for legacy ?q= URLs
Status: Fixed » Active

That's great! We should still add a change notice for this though, or update the existing one that mentions the index.php/ style ones.

Crell’s picture

Title: Change notice: Provide redirects for legacy ?q= URLs » Provide redirects for legacy ?q= URLs
Status: Active » Fixed

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