I noticed that the implementation of hook_token_values() check if the $type parameter is also equal to 'all', and the switch statement includes a default statement that executes the same code executed when $type is equal to 'taxonomy'.

hook_token_values() should never get such value for $type, and the code executed when $type is not equal to 'node'could receive an $object that doesn't have a tid property (this could happen if token.module defines a new value for $type, or a third party modules defines a new token category).

CommentFileSizeAuthor
#5 370137-pathauto-token-values-D6-2.patch10.6 KBdave reid

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Active » Postponed (maintainer needs more info)

I think it should be possible to get "all" #173699: hook_token_values should handle "all" ?

Is there a specific bug related to this or it just seems bad to you?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Postponed (maintainer needs more info) » Active

There is a module that defines a new group of tokens, and then it calls token_replace() passing its custom object (and using for $type a value that is not normally used from token.module).
I don't remember the name of the module, but I used it for some time in my local site; I uninstalled it more than one month ago.

I guess that using that module pathauto.module would try to access a property that doesn't exist for the custom object used by the module.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Version: 7.x-1.x-dev » 6.x-1.x-dev

The problem is this logic:

function pathauto_token_values($type, $object = NULL) {
  if (module_exists('taxonomy')) {
    if ($type == 'taxonomy' || $type == 'node' || $type == 'all') {
      _pathauto_include();
      switch ($type) {
        case 'node':
          // node tokens stuff
        case 'taxonomy':
        default:
          // taxonomy stuff
    }
  }
}

If I use type = 'user', it's going to trigger the default condition and try to get taxonomy tokens. Luckily there's some built-in checking, but the whole logic of this seems like it could use some cleanup.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

It would probably be better if pathauto_token_values('node') wants the taxonomy tokens for a node, to call pathauto_token_values('taxonomy') from within itself and merge the tokens together. That's a little easier to understand and follow than the switch/case/fallback code.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Version: 6.x-1.x-dev » 6.x-2.x-dev
Status: Active » Needs review
StatusFileSize
new 370137-pathauto-token-values-D6-2.patch10.6 KB

Here's my patch for revamping the implementation of pathauto_token_values(). Also includes tests for the [catpath/catalias] tokens, working on getting tests for the [termpath/termalias] tokens.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

And we shoudn't be allowing 'all' in pathauto_token_values(). As per the token module implementations, it has to be specifically 'node' or 'taxonomy', etc.

Anonymous’s picture

Anonymous (not verified) commented

The patch resolves the problem I pointed out, and it also adds more tests for the code.
all should not be used as $type because token.module doesn't pass that value to hook_token_values(), nor it will never pass such value (what object should then token.module pass in this case?).

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Assigned: Unassigned » dave reid
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Fixed

Committed to both Drupal 6 branches, working on up-porting to the Drupal 7 branch.
http://drupal.org/cvs?commit=341090
http://drupal.org/cvs?commit=341102

Status: Fixed » Closed (fixed)

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