Pathauto is currently using a bunch of strtolower(), strlen(), and other string functions that have drupal_ equivalents. We should probably use the drupal_ version for most (if not all) of these instances. (If someone gets around to it before I do, here's a hint: Use Coder to find the instances. :))

I'm also going mark this as blocking the release of 6.x-1.1.

CommentFileSizeAuthor
#1 pathauto_use_drupal_str_functions.d6.patch2.65 KBFreso

Comments

Freso’s picture

Freso commented
Status: Active » Needs review
StatusFileSize
new pathauto_use_drupal_str_functions.d6.patch2.65 KB

Is there a reason these instances were not using the drupal_ functions? (Attached patch has not been tested.)

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

No reason that I know. I think I just didn't know about the drupal_*str* set of functions. This seems RTBC to me.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Reviewed & tested by the community

I gave this some testing and, though I found some other bugs, I don't think this introduces any bugs (my testing and the simpletests still work as expected).

Freso’s picture

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

Alright then, committed. :) It should probably be backported though.

Freso’s picture

Freso commented
Status: Patch (to be ported) » Fixed

The patch more-or-less applied to 5.x (one failed hunk, some offset), so committed there as well.

I also stumbled upon an empty for-loop while applying the failed hunk:

    for ($i = 0; _pathauto_alias_exists(drupal_substr($alias, 0, $maxlength - drupal_strlen($i)) . $separator . $i, $src); $i++) {
    }

This should probably be investigated for another (clean-up =)) patch... (It exists in both D5 and D6 versions.)

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I think the empty for loop is just a way to keep trying new aliases until we get one that's unique. It's perhaps an overly fancy/brief construction which is difficult to understand (I didn't write it and I still have to think about what it does to see if it works).

Freso’s picture

Freso commented

From what I can see, nothing is done during the loop - not while calculating the criteria, not while actually running it! If it does anything, it's using time/CPU cycles. But then, perhaps you're seeing something I'm not. ... And then I realised it's making and storing a count in $i, which is used just a bit later... This is definitely worthy of a code comment. Before we forget what it is. :)

Freso’s picture

Freso commented

And since $i will always be an integer, we can safely use PHP's own strlen on it, for speed. So I'll reverse this bit.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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