pathauto_cleanstring() does not convert to lowercase (consolidate all text altering code to pathauto_cleanstring)

Ready for community review.

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?

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?

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.

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.

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...)

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.

I tested the patch, and it works for me.

Patch works fine on latest -dev

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.

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.

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!

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

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

subscribing

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.

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.

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

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...

Agree!

@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

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.

@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.

Thanks greggles!

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.

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