Port PURL to Drupal 7

subcribe

Sub

subcribe

subscribe

sub

Here's a first pass at this thanks to coder & coffee. I hope the coding standards upgrades were ok to throw in, the online coder upgrade tool didn't work when making a separate coding standards pass for some reason...so I mixed it in...

Things seem to work (or rather don't break everything), forms, tests sent some modifiers to the menu items...

TODO
- Implement the new Language API as needed???
- Find something that can use this or make more tests (Spaces anyone?)
- Sort out the tests (so Simpletest can see them)
- Use the new drupal_static where possible
- Make sure the _purl_form_alter() is looking in the correct spots for ['menu-name'] in menu_edit_menu & ['options'] in menu_edit_item

Cheers,
JP

OK second pass at this,

- Tests are fixed (they test things)
- Fixed some code style stuff & found another item to upgrade in purl_querystring.inc
- Tests show that adding a provider for anything other than Path modifiers is busted, don't know why yet, any insight appreciated

TODO
- Figure out if language_initialize still affects $_GET['q']
- Use the new drupal_static where possible
- Change any old DB methods to new API (db_delete @597)

@jpstrikesback, I've applied your patch from #7 and created a 7.x-1.x branch from that. Most of the functionality (at least, the path-based stuff) is working. The tests still had some errors, but this is a really good start. Thanks for the patch.

Commit: http://drupalcode.org/project/purl.git/commit/411d79d

@tobby, Awesome! I'll try and get more into the test failures as the weekend permits : )

subscribing

subscribe

K

The first reason it's failing is that in purl_modifiers() when caching the modifiers it only caches the first method requested...which likely means the 6.x dev branch is having some fun there as well.

Once I removed the modifier cache then I needed to patch up the purl_querystring->rewrite() method to make any empty $options['query'] array a string and not touch it after that, i.e. I found an errant 6.x item (drupal_query_string_encode()) and was replacing with newer stuff (drupal_http_build_query()) but realized it actually wasn't required and was breaking stuff cause once it runs thru url() it get's treated.

Patch forthcoming with or without purl_modifier caching...

And the patch. All tests pass for me, I'm interested in why $_GET['q'] and $_REQUEST['q'] don't hold the same values at hook_init() in D7 but it works so whatever. This one is without the purl_modifier cache as that needs some work...
Cheers,
JP

EDIT: Note: This patch applies to the 7.x-1.x branch.

Found a new issue it seems:

- when using the Purl Test Module while under the influence (of say /foo), rewrites work great everywhere but when going to the front page by clicking the "Home" icon, this produces a 404 as it tries to send us to /foo/<front> instead of /foo/node (which is set as front)

@jpstrikesback: This patch should apply (at least before your latest) to 7.x.

http://drupal.org/node/684572#comment-4276672

subscribing

@glennpratt, applied & tests pass, thanks! I've rolled a patch with the fixes from #13, from http://drupal.org/node/684572#comment-4276934 & fixes to issues with <front> mentioned in #14.

Regarding #14, the reason this was happening is that in D6 custom_url_rewrite_outbound() is called directly after '<front>' is rewritten to '', in D7 this little rewrite happens after hook_url_outbound_alter() is invoked and so when a path modifier is present Purl changes the path and the '<front>' string in path is now something like 'somepath/<front>' which of course get's missed by the little check for equality in url()...so I stuck the same thing inside purl_url_outbound_alter():

  if ($path == '<front>') {
    $path = '';
  }

which means that nothing can react to that special path string down the line, which is perhaps inelegant (but maybe no less than '<front>' itself)...thoughts anyone?

oh yeah...the patch...

carp, one more time, and this time with the right patch...

Subscribing

For anyone who was like me and didn't know where to find the D7 branch to which these recent patches apply, visit the following link and click "snapshot":

http://drupalcode.org/project/purl.git/tree/refs/heads/7.x-1.x

Okay, I applied the patch at #19, and then enabled the module, but all I get is a redirect loop on any page except the homepage. Any idea why?

@joelstein - can you paste the contents of your purl_init()?

Sure:

/**
 * Implements hook_init().
 * Checks for any valid persistent urls in request string and fire callback appropriately
 */
function purl_init() {
  static $once;
  if (!isset($once)) {
    $once = TRUE;
    // Initialize a few things so that we can use them without warnings.
    if (!isset($_GET['q'])) {
      $_GET['q'] = '';
    }
    if (!isset($_REQUEST['q'])) {
      $_REQUEST['q'] = '';
    }

    // Initialize the PURL path modification stack.
    purl_inited(TRUE);

    // TODO - updated function for the new API but is it ever going to be needed by something??
    // $_GET['q'] = $q = purl_language_strip($_GET['q']);
    $_GET['q'] = $q = $_GET['q'];
    drupal_path_initialize();
    purl_get_normal_path($q, TRUE);
  }
}

huh...looks like the patch applied...

1st see my question in #13.

This is the behaviour I noticed at the beginning and it was just that $_REQUEST['q'] was empty (strange that GET & REQUEST weren't the same?) so I put $_GET['q'] in it's place and all was well (i.e. the path requested was in $_GET['q']), try putting $_REQUEST['q'] back like so:

$_GET['q'] = $q = $_REQUEST['q'];

See purl_get_normal_path() to see why it is deciding you get to go to the front for everything.

I'm very curious about this cause it is perhaps server related.

I don't know much about how $_GET and $_REQUEST get set in D7.

I changed $_GET['q'] = $q = $_GET['q']; to $_GET['q'] = $q = $_REQUEST['q'];, but now every page automatically redirects me to /node.

See purl_get_normal_path() to see why it is deciding you get to go to the front for everything.

I think you misread what I wrote. It wasn't taking me to the front page for every request. The front page loaded just fine. However, any other non-front page would just give me a redirect loop in my browser. Make sense?

Would you mind attaching the latest version of the module here as a zip, in case I patched something incorrectly?

Aack I did misread, apologies.

My latest is attached, it should match 7.x with the patch from #19 applied.

If you are getting a endless redirect then I haven't experienced that yet. Try the version I've uploaded and if it's still happening try running the tests and let me know which ones fail.

Cheers,
JP

Thanks... I dropped your module in, re-enabled, and I'm still getting the redirect loop. Can't run the tests because... I'm getting a redirect loop. :(

However—I just created a fresh install of D7's latest dev, and then only installed your PURL module and Ctools (7.x-1.0-alpha4), and now I don't get the redirect loop.

So, it looks like it's something unique to my setup. I'll investigate further and share what I find.

Interesting, I'd love to know what caused it when you find out. By chance were there any modules in place using purl_goto()?

Nope. After more digging, it appears to be caused by having both Persistent URL and Global Redirect (7.x-1.x-dev) enabled. Either can be installed without the redirect loop. Install them both and the redirect loop appears. Can you take a look and see what would cause this integration problem? Is it possibly a module weight issue?

Possibly related:

• http://drupal.org/node/819998#comment-3082288.
• http://drupal.org/node/900622
• http://drupal.org/node/623886#comment-2562854

Also, I got an error when using the purl_save() and purl_delete() functions, which are resolved by the attached patch.

http://drupal.org/node/900622 without getting into it too much it looks like this is the one to watch, from the description there it would seem that ATM the two are incompatible because of what Global Redirect does to $_GET_['q'], but maybe weighting it after PURL could help?

PURL is already lighter than Global Redirect.

To address your question in #13 (from http://drupal.org/node/877696#comment-3401302):

...in Drupal core the convention has been to leave $_REQUEST['q'] alone. For example, all of the language-based path handling rewrites $_GET['q'] stripping out language prefixes (e.g. es/node/5 becomes node/5) while $_REQUEST['q'] is left alone. The same methodology is used for path aliases. This convention has been relied upon across contrib (in i18n and/or path alias related modules particularly) to determine whether the current path in $_GET['q'] has been modified as a result of something like a prefix being stripped or the current path being aliased.

One other thing I noticed is that URLs with path aliases aren't getting rewritten. Is this because hook_url_outbound_alter (where PURL rewrites the URL) happens before path aliasing does in url()?

http://api.drupal.org/api/drupal/includes--common.inc/function/url/7

Notice hook_url_outbound_alter() gets fired, then shortly after that the $path variable gets overwritten with the alias.

Perhaps another way to do this is to rewrite the $options (instead of the $path) by setting the $options['prefix'] value (see attached patch). Thoughts?

The purl_goto function shouldn't wrap the options array in another array when calling the url function. Submitting a patch for this small fix.

I've also noticed that purl_save makes a call to purl_load (line 598) with the $modifiers argument being NULL, causing the foreach loop to fail. I haven't fixed this yet, since I don't know exactly what $reset does.

Here's a patch which applies to the D7 branch, and incorporates the patches from #19, #29, #32, and #33 above.

@joelstein & ojohanssen, cool, I'll get a chance to play/test very soon! Do tests pass?

I haven't run the tests. How do I do that?

To run tests in D7 enable the core "Testing" module, then enable "Persistent Url Tests" and head to:

/admin/config/development/testing

Then check off the tests you want to run (i.e. Purl) and click "Run Tests" at the bottom.

Cheers,
JP

Pretty cool! You should probably make Testing a dependency of Persistent URL Tests.

The patch in #34 causes 9 tests to fail. However, if I remove the code added in #32, they all pass.

We really need something for the issue reported in #32. In D6, custom_url_outbound_rewrite() was called after the path alias was set. However, in D7, hook_url_outbound_alter() is called before the path alias is set. In other words, URLs which are rewritten by PURL will be rewritten again by url(), thus losing the path prefix.

That's why I think a good solution would be to put the path prefix in $options['prefix'], which works in my manual site testing, but somehow fails the automated tests.

Any ideas?

Subscribe

sub..

Following.

subscribe

I was getting a notice from purl after editing and saving a menu item:

Undefined index: menu_name in purl_item_edit_submit() (line 311 of purl/purl.admin.inc).

This patch fixes that. The modifier does seem to be saved at all, so will need to address that.

(This patch was applied to code that had the patch in #34.)

+1

I've created a fix for my other issue in comment #33:

"I've also noticed that purl_save makes a call to purl_load (line 598) with the $modifiers argument being NULL, causing the foreach loop to fail. I haven't fixed this yet, since I don't know exactly what $reset does."

I've studied the code, and I can't figure out why purl_load() is called from purl_save() and purl_delete() without specifying any modifiers. So what I've done, was simply to remove those calls. They aren't used anywhere else in the module.

Somebody should merge the #43 and #45 patches into the comprehensive patch in #34.

I would but I'm waiting for the maintainers to weigh in here (no pressure or anything, just wondering on thoughts from P2 peoples on the direction they are taking this baby)

Hi rerolled all patches. I had some problems with query strings in $options['query'] in purl_goto(). $options['query'] must be an array but some implementations pass an string.

Please review.

sub

scribe.

Finally all tests pass. Reworked the patch. Development sandbox at http://drupal.org/sandbox/webflo/1179340

subscribing

subscribe+

I was testing the purl module on d7 but i'm getting a foreach error upon creating a new purl. In the purl_save function purl_load is called with NULL as $modifiers. If you look at the purl_load function, the function starts with a foreach of $modifiers. Don't really understand why purl_load function is in the purl_save function like this.

@Takki

There is an easy fix for this in #45. It has worked fine for me ever since.

Thanks ojohansson. I must have skipped your patch, all is fine now.

subscribing

subscribing

This patch for the D7 version doesn't place nice with the globalredirect module though, an issue PURL has had before in Drupal 6: #623886: Conflicts with globalredirect. Is there anything we can do on this side?

I was unable to clone this with git, getting a "remote HEAD referes to nonexistent ref".

Any ideas?

My sandbox has no master. Try git clone --branch 7.x-1.x http://git.drupal.org/sandbox/webflo/1179340.git

Is there any reason why there has not been a 7.x-1.x-dev release yet. I see there is 7.x-1.x in git and webflo has done some work in his sandbox. I don't see any reason why we cant merge all the work done here and do a dev release and close this issue.

Is there any reason why there has not been a 7.x-1.x-dev release yet. I see there is 7.x-1.x in git and webflo has done some work in his sandbox. I don't see any reason why we cant merge all the work done here and do a dev release and close this issue.

subscribing

I added a dev release so we can now file issue and patches against it. I would be happy to integrate the work webflo has done as a starting point.

Woohoo!

sub