Code (style) normalisation

You have a $patern = ''; which should be $pattern = '';. I'll leave this for Greg to review properly though, as he may know of a reason why the trims are done the way they are. (Also, you can find more (trailing) whitespace if you use Coder. ;))

Patch doesn't apply cleanly anymore. Please re-roll.

The if (!$pattern) checks are not E_NOTICE compliant, which is something that a "code style" patch should also fix.

I'm also not sure that moving the trim() above is any clearer nor am I really sure that it's necessary...so, perhaps we can just drop that and change these to

if (isset($pattern) && $pattern) {

The whitespace changes seem fine to me.

This won't do: watchdog('pathauto', $update_msg);. watchdog() should be fed the "raw", untranslated string and then handle the translation when displaying the message in the logs. I'm not sure how to handle plurals within watchdog() though, so this might have to be looked into.

Also, I'd vote for waiting with committing this for 6.x-2.x.

D'oh! Commented on wrong issue. My bad. :x

Okay, here's the deal Greg and I just agreed on:
We want the variables stored with trim(), ie. $pattern = trim(...).
We want to use !empty() to check that the variable exists (and contains something), ie. !empty($pattern). This should also remove the need to initialise the variables before time, right?

Please let me/us know if we're out of our minds! :)

This is looking good. Thank you for your work on this! :)

If you're up to the task, there are some instances of if (isset($var) && $var), and changing these to if (!empty($var)) would be a nice addition to this code style clean-up. :) (If not, that's alright too, and this patch is close, if not ready, to being committed.)

Hm. I'm not completely sure what you mean... ? Both if (isset($var) and $var) and if (isset($var) && $var) should be if (!empty($var)), as far as I can tell... but yeah, if there are any and's floating about outside of such a statement, they should probably be made to &&'s, for consistency. :)

Right, that should be changed to && (for consistency). :)

Oh, and good catch!

Just a note: pathauto_user() has $new_user->roles = isset($edit['roles']) ? $edit['roles']: array(); which should have a space before the colon. (This is only in the 6.x branch.)

A couple of hunks didn't apply anymore, so I re-rolled and incorporated some of the changes mentioned above.

Looks good to me.

While reading through the code again, I stumbled across this: if (!isset($node->pathauto_perform_alias) || $node->pathauto_perform_alias)
"If $var is not set OR if it is true." Is this necessary? There are only ever so few cases where $node->pathauto_perform_alias won't fulfill either of those states... so I'm thinking that either one of them could probably go, or they both could... ?

Similarly, instances like if (isset($old_alias) && drupal_strlen($old_alias)) could probably be replaced with if (!empty($old_alias)) as well, as the only time drupal_strlen($old_alias) will return something other than 0, is on a non-empty string. Thus, this should theoretically be an ever so slight performance increase as well. Any objections/reasons not to do this? :)

An updated patch (without changing anything mentioned above) attached, which also includes some double to single string conversion.

The pathauto_perform_alias stuff is very twitchy - it took a long time to work out a bunch of edge cases (user creates/edits nodes and does/doesn't have permission to create path aliases and node has a pattern set/doesn't but follows default). So, I'm hesitant to change that...

The second item you point out makes sense to me.

Since empty($var) returns true if $var is '0' (ie, a one character string containing the character "0"), we might not want to do this. But then, why not? If people "hardcode" their patterns to be "0", I'd say they're asking for trouble anyway. The only problem would be if people use something like "[title-raw]" (ie., where the title (or another, single token) is all there is in the pattern), where the title happened to be "0" - which might just happen. So I'm unsure of what to do. (I'll try and do some testing to see the actual results with and without this patch.)

But here's an updated patch; including a re-roll, some array code style fixin', and the empty() mentioned in the second part of comment #21. Also, I might commit a temporary patch based on this, without the empty() changes. I'll make a comment here if I do.

Eh. And the patch.

Huh? Hm... third time's the charm, no?

Hm. It seems I'm being hit by #267884: Attachments failing when users preview or use "attach" button... so I'll try again.

Edit: Yay! :D

Okay, here's the deal on using empty(), from what my quick testing revealed.

• My assumption was correct. Using "[title-raw]" and then "0" in a title won't result in an alias being created with the current patch.
• However, without the patch, even if the alias is created, Drupal will still rather use "node/X" than "0". If specifically asked to fetch "0", it'll do so, but it won't use it on "node/" or similar places using links. I have a feeling this should be reported as a bug with Drupal though.
• Using only the title in the pattern and only having "0" in the title is quite an edge case, I'd dare say (even for taxonomies, forums, etc.). However, I am undecided on whether to cater for the edge case, or whether to simplify the code... perhaps a helper function _not_zero (or something) to do both?

About bullet #2 above, check #268909: "0" can't be used a path alias, but no error is shown for development on this. :)

Anyway, the test returns no errors with current patch, so I committed this. If the empty()'ies turn out to be problematic, we can weed through which ones are causing problems and try something else (if (empty($var) && strlen($var) < 1) perhaps?).

Also, we should do this normalisation on the 5.x branch as well.

I love how patches apply almost 1:1 between 6.x-1.x and 5.x-2.x. :)

If nobody is quick to step up and object to this, I'll apply it soonishly.

And... committed.