I pointed out some trouble I was having with project_release tokens to greggles in IRC, and he called it a pathauto bug. ;) pathauto_cleanstring() is only called on tokens, not the finished alias. So, if you have a trailing separator (e.g. from using a token that only conditionally has a value), the trailing separator is left in the resulting alias if that token happens to be empty for a given node.

Patch coming soon to add pathauto_clean_alias(), which (at greggles's scope-creep request invokes hook_pathauto_clean_alias(), too). ;) Stay tuned.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Status: Active » Needs review
FileSize
2.95 KB

Note, since both pathauto_cleanstring() and pathauto_clean_alias() want to strip leading/trailing separators and collapse duplicates, I moved that code into a private helper function that's shared by both.

Also note that pathauto_cleanstring() should be renamed to pathauto_clean_string(), but that's out of scope for this issue. ;)

greggles’s picture

I'm inclined to commit this but leave it here for review for a few days.

Regarding the new hook and why scope creep is ok: It will allow us to say "oh, you want to do crazy? yeah, do it yourself based on hooks." Instead of just arguing over what we are/aren't going to do.

dww’s picture

I'm all in favor of hooks. I just didn't want anyone to accuse me of slipping in a new feature with my bug unprovoked. ;)

LinL’s picture

Hi,

I'd run into a similar issue and was about to add a feature request/patch.

My alias pattern is something like this:

1/[field_name-raw]-[field_optional1-raw]-[field_optional2-raw]

I had been putting whitespace in the pattern, instead of the separator, and then had a small patch to pathauto.inc to trim and replace whitespace in the complete alias.

Now I've tested the patch in #1 instead and it works well for me. :)

greggles’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Patch (to be ported)

Thanks dww for this patch.

Applied to 6.x-1.x http://drupal.org/cvs?commit=327030

@dww Any chance you can port this to 7.x?

dww’s picture

Assigned: dww » Unassigned

Thanks for committing! Sorry, no chance for me to port this anytime in the near future, no. :( My plate is already completely overflowing...

Dave Reid’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.86 KB

D7 patch attached for review.

Dave Reid’s picture

Status: Needs review » Needs work

Revised and committed to 7.x-1.x and 6.x-2.x as well.

Something I'm not exactly sure on is why the lowercasing is in a function meant to remove separators. Also, we need to document this in pathauto.api.php.

Dave Reid’s picture

Priority: Normal » Major

This inconsistency needs to be fixed before the next release on any branch.

Dave Reid’s picture

Title: Leading and trailing separators (and duplicate separators) are not stripped from aliases, only tokens » Fix pathauto_cleanalias() and pathauto_cleanstring()
Status: Needs work » Needs review
FileSize
8.8 KB

I'm really not happy with the hook that was added here.

  1. I don't think it makes sense to be invoked in the context of pathauto_clean_alias(). The purpose of this is to alter the alias that pathauto is generating, then it should be renamed to hook_pathauto_alias_alter() and be invoked from pathauto_create_alias().
  2. Lowercasing was moved from pathauto_cleanstring() to pathauto_cleanalias(). Again does not make sense.
  3. If we really wanted a pathauto_cleanalias() then the rest of the string cleanup functions from pathauto_create_alias (e.g. removing duplicate repeated slashes, trimming leading or trailing slashes) should have been moved there as well.
  4. Any kind of 'url' tokens should not have pathauto_cleanstring() applied. It should always be assumed that if URL tokens are used in a pattern we only want to check for duplicate slashes around alias once the token has been replaced.

Status: Needs review » Needs work

The last submitted patch, 545216-pathauto-fix-cleanalias-D7.patch, failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
8.03 KB
Dave Reid’s picture

Status: Needs review » Needs work

The last submitted patch, 545216-pathauto-fix-cleanalias-D7.patch, failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
9.26 KB
Dave Reid’s picture

Dave Reid’s picture

With a test creating a parent term with the alias 'My Crazy/Alias' and a child term with the pattern [term:parent:path]/[term:name], so it would end up as 'My Crazy/Alias/child-term'

Status: Needs review » Needs work

The last submitted patch, 545216-pathauto-fix-cleanalias-D7.patch, failed testing.

Dave Reid’s picture

Dave Reid’s picture

Assigned: Unassigned » Dave Reid

Still hoping that core bug gets fixed shortly, but don't think I can do much until then. :/

Dave Reid’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 545216-pathauto-fix-cleanalias-D7.patch, failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
12.16 KB

Let's try that without the debugging line and a stupid logic error in the test...

Status: Needs review » Needs work

The last submitted patch, 545216-pathauto-fix-cleanalias-D7.patch, failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community

Did a contrib-wide search for implementations of hook_pathauto_clean_alias and found none. Committing shortly.

Dave Reid’s picture

Version: 7.x-1.x-dev » 6.x-2.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 7.x-1.x CVS. http://drupal.org/cvs?commit=404100

Dave Reid’s picture

Status: Patch (to be ported) » Needs review
FileSize
14.19 KB
Dave Reid’s picture

Try this again with PHP 5.3 fixes...

Dave Reid’s picture

Dave Reid’s picture

Version: 6.x-2.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

Simpletest for D6 is getting really irritating with not installing dependencies. This passes absolutely fine locally.
Committed to 6.x-2.x: http://drupal.org/cvs?commit=404400

Dave Reid’s picture

Status: Patch (to be ported) » Fixed

Committed to 6.x-1.x as well after manual testing: http://drupal.org/cvs?commit=404428

Status: Fixed » Closed (fixed)

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

cyberderf’s picture

Sorted ?