pathauto_cleanstring provides a function to clean string values provided by a module. However, previously it failed to convert a string to lowercase even when that option was set in the pathauto configuration.

The attached patch to pathauto.inc enables that functionality, using a minor modification to the code from pathauto_create_alias that is used by pathauto itself.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

babbage’s picture

Assigned: babbage » Unassigned

Ready for community review.

apaderno’s picture

Title: pathauto_cleanstring does not convert to lowercase » pathauto_cleanstring() does not convert to lowercase
greggles’s picture

Status: Needs review » Needs work

It took me a while, but I think I understand this now.

For modules _other_than_pathauto_ that use pathauto_cleanstring and tell their users that the administration settings of Pathauto will affect their strings, it is confusing that the lower case option does not in fact make the strings lower case.

To be consistent, all the string cleaning in the Pathauto admin area should be enforced inside of pathauto_cleanstring. Maybe there was a reason at one point why this was in the alias creation but I think now that it should be placed into the cleanstring.

So, I'm marking this as "needs work" since we need to remove it from the pathauto_create_alias to centralize it in one place. Do you agree that makes sense?

babbage’s picture

Ah. The commented description of pathauto_cleanstring ("Clean up a string value provided by a module") had led me to wrongly conclude that this function was being provided as a service to other modules but not being used internally by pathauto itself, since I'd found string cleaning code elsewhere, as you note. I see now that it of course is being used by pathauto as well.

I am in two minds about whether this should be rolled into one patch. It seems to me the original patch is a perfectly valid patch for the issue originally identified, and weeding out all string cleaning code elsewhere to consolidate it all into this function is a larger job that might be better to be treated separately. For instance, pathauto_create_alias also collapses two or more slashes into one, trims leading or trailing spaces, and truncates a url to a maximum length, all of which could be considered string cleaning. Moving all this code into pathauto_cleanstring could potentially have effects in other parts of the module and while it is sensible work to do I come back to whether it is really a separate issue to the original patch I proposed?

greggles’s picture

Well, AFAIK the only string cleaning code that needs to be moved is this one feature. I don't want to create a separate issue just for that one little fix.

babbage’s picture

Assigned: Unassigned » babbage

OK. I thought you were proposing moving any kind of string cleaning. I will re-roll the patch later today with the change you've suggested when I am back at my development machine.

babbage’s picture

Status: Needs work » Needs review
FileSize
1.27 KB

Re-rolled against HEAD, incorporating the suggestion made by greggles in #5. Very simple patch.

In this patch I also cleaned up an error in a nearby comment, which had a text fragment that made no sense. (If the end of that fragment was important, it may need to be retrieved from an earlier version of the code? In the meantime, didn't seem useful having a nonsensical fragment...)

babbage’s picture

Assigned: babbage » Unassigned
babbage’s picture

Status: Needs review » Reviewed & tested by the community

This is a simple patch and has been sitting unreviewed for a while... I'm going to mark this RTBC in the hope of getting this committed or at least reviewed by a maintainer.

apaderno’s picture

I tested the patch, and it works for me.

thePanz’s picture

Patch works fine on latest -dev

greggles’s picture

Status: Reviewed & tested by the community » Needs work

This failed against 6.x-1.x. I guess it would fail against 5.x-2.x too.

@dbabbage - I know it's small but if you could create patches for all three versions I'll commit them. Thanks for your work on this.

jared12’s picture

I'm new to this whole "patching" concept, but it appears to have worked fine on the current 6.x-1.x-dev (2009-May-03) version. It would be great if this patch was committed.

Although my whole purpose for using it doesn't seem to be working anyway. I was trying to get this line of code:

php: return 'users/'.pathauto_cleanstring(token_replace('[user]', 'user', $user));

to work for me in the IMCE Directory Path settings. (I found it here.) If I ever figure out how to make it work, this patch is exactly what I'll need.

jared12’s picture

Everything's working perfectly now. I got some help, and discovered I needed to use this line in IMCE to make use of pathauto_cleanstring:

php: _pathauto_include(); return 'users/'.pathauto_cleanstring(token_replace('[user]', 'user', $user));

Thanks for this patch!

MGN’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
1.54 KB
1.24 KB

Reroll of #7 for 5.x-2.x-dev, 6.x-1.x-dev, and 6.x-1.x-dev, as per greggles request.

MGN’s picture

Oops. Forgot about that pesky comment fragment in d52 and d62....try these instead.

divThis’s picture

subscribing

Kane’s picture

Will this patches be on next updates of dev version? I'm not sure that i'll remember to patch next version when it comes.
By the way, thanks for patches.

vthirteen’s picture

patch < pathauto-inc-lowercase-d61_0.patch 
patching file pathauto.inc
patch unexpectedly ends in middle of line

then i patched it manually, but i can't see any difference. deleted the cache and reset the module, but the path (used by custom breadcrumb module) still uses upper-case letters.

greggles’s picture

You will have to edit the node to get it to regenerate the alias (and make sure your update action includes updating the alias...).

vthirteen’s picture

thanks, but my problem is not with the path of the page but with the link generated by pathauto for custom breadcrumbs. and saving again the page does not affect the uppercase in the breadcrumb path.

e.g. for a page "Page Title" that belongs to Taxonomy-Term-A where pathauto uses [vocab-raw]/[title-raw] and where Custom Breadcrumbs uses <pathauto>|[vocab-raw], the path of the page is /taxonomy-term-a/page-title (correct) but the breadcrumb path is /Taxonomy-Term-A (upper case)

my guess is that Custom Breadcrumb is not picking the right variable...

sp_key’s picture

Agree!

MGN’s picture

@vthirteen - its best to decouple these issues. First, verify that the patch corrects the url case problem.

At /build/path/pathauto set the Character case option to 'Change to lower case.'

Create a test script such as a page with input format set to "php code".

Then add a little script that will test the change of case

module_load_include('inc', 'pathauto');
$path = 'My Uppercase Path';
$clean = pathauto_cleanstring($path, FALSE);
$message = "path is " . $path . " and cleaned is ". $clean;
print $message;

Before applying the patch you will get something like (depends on what you are using for a separator)

"path is My Uppercase Path and cleaned is My-Uppercase-Path"
obviously broken

After applying the patch you will get

"path is My Uppercase Path and cleaned is my-uppercase-path"
The expected behavior!

If you can confirm the patch results in the correct behavior, then we can move on to your custom breadcrumbs problem (start a new issue on the custom breadcrumbs queue).

I hope this helps move things forward. I haven't checked to see if the patches are out of date again or not. If they are I can look into re-rolling yet again, assuming greggles is still interested.

greggles’s picture

Title: pathauto_cleanstring() does not convert to lowercase » pathauto_cleanstring() does not convert to lowercase (consolidate all text altering code to pathauto_cleanstring)
Status: Needs review » Fixed

@MGN - you are absolutely right - thanks for your comments :)

Now applied to 6.x-1.x http://drupal.org/cvs?commit=278076 and 6.x-2.x http://drupal.org/cvs?commit=278072.

I'm not really maintaining 5.x any more and the latest patch for that fails. If someone re-rolls I'll commit it.

MGN’s picture

Thanks greggles!

vthirteen’s picture

thank you MGN and greggles and sorry if i made you waste your time. indeed i had not applied the patch properly. just updated to the new dev version of the module (Oct 22) and it works as expected.

now the problem is apparently with Custom Breadcrumbs. i'll open a new issue there.

EDIT: the patch recently committed and included in the dev versions indeed solves the problem. case closed.

Status: Fixed » Closed (fixed)

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