http://drupalcode.org/project/xmlsitemap.git/blob/refs/heads/7.x-2.x:/xm...

This uses hook_entity_info_alter() to define its entity. Problem is, token module also uses hook_entity_info_alter() to fill in token types for entities.

So token doesn't get to fill this in for the entity defined here, and so it's missing a token type.

Therefore, it causes modules that assume the token type to always be there if token module is there to show errors: #2197041: PHP notice after clearing the cache: Undefined index: token type in flag_token_info().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Honza Pobořil’s picture

So what solution do you recommend?

joachim’s picture

Either use hook_entity_info() instead of hook_entity_info_alter(), or provide the token type.

DamienMcKenna’s picture

As XMLSitemap does not depend upon the Token module, I don't think it's strictly a bug in XMLSitemap.

joachim’s picture

If you used hook_entity_info() instead of hook_entity_info_alter() then Token module would be able to act and provide the token type for you.

Pafla’s picture

How and where to edit this?

dandaman’s picture

Would it also fix it if we changed xml sitemap to run the alter before token then?

Shawn DeArmond’s picture

Status: Active » Needs review
FileSize
946 bytes

@dandaman's solution worked for me. I changed the module weight to -1, and it passed all tests.

Attached is a patch that implements it.

PLEASE REVIEW because there might be other consequences of changing module weight.

daveferrara1’s picture

Having the errors too. Mainly triggering this error:

Notice: Undefined index: token type in flag_token_info() (line 88 of /srv/bindings/9fe9a2846c534faab85e3b36c93bb52c/code/sites/all/modules/flag/flag.tokens.inc).

daveferrara1’s picture

Patch in #7 worked for me.

Grimreaper’s picture

Hello,

I attach a patch that add a 'token type' key in the hook_entity_info_alter.

I don't test if it is the proper solution but it solved the warning.

I don't think the patch in #7 is a good solution because it only hides the problem. And the hook_update_N in this patch is for a D6, no ?

Thanks for the review.

joachim’s picture

Thing is, this is going to happen with any entity info property that another contrib module wants to add to all entities and then rely on.

The real problem is that the hook is being misused: alter hooks should not add a whole new item to an info array.

Dave Reid’s picture

The problem is that we're registering an entity type for a table provided by core. What happens when another contrib module wants to declare menu links as entities and does the same thing? *boom* This is why the alter hook is used currently because this is not a theoretical problem at this exact moment.

joachim’s picture

Yeah... contrib projects need to agree amongst themselves who gets to entitize a particular core table. Or declare on their project pages that they're mutually incompatible.

justanothermark’s picture

FileSize
514 bytes

I agree this is a wider problem & not sure of the fix but if the workaround in #10 is to be used then it should use the key 'token type' (as mentioned in the comment) and not 'token info' (as used in the patch).

Grimreaper’s picture

Hello,

Thanks for the review, you're right justanothermark, it's 'token type'. I tested at work, and I rewrote a patch at home and I didn't realize I wrote a wrong key.

In the token module in the hook_entity_info_alter :

 // Fill in default token types for entities.
    switch ($entity_type) {
      case 'taxonomy_term':
      case 'taxonomy_vocabulary':
        // Stupid taxonomy token types...
        $info[$entity_type]['token type'] = str_replace('taxonomy_', '', $entity_type);
        break;
      default:
        // By default the token type is the same as the entity type.
        $info[$entity_type]['token type'] = $entity_type;
        break;
    }

So the value should be 'menu_link'.

Here is a patch with the right key and value.

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community

Applying the patch in #15 to the latest dev has caused the notices to cease for me.

kerrycurtain’s picture

Confirming the patch in #15 worked for me, Thank you.

rooby’s picture

Yeah... contrib projects need to agree amongst themselves who gets to entitize a particular core table. Or declare on their project pages that they're mutually incompatible.

Entitizing (funny word :)) a core table should not be done by some random module that needs to use that data as an entity, because if you try to do it that way then you are relying on some other unrelated module that a user may not want to use.

Ideally it should be in a module that specifically does just the entitizing and then is required by other modules that need it.

So in this case some sort of menu_item_entity module would handle just this entity definition and any other modules that need it require it.

This seems to be kind of what entity_menu_links does although it also has some things that a straight entitizing module doesn't really need, like the UUID/services stuff so it isn't really a candidate for a dependency of xmlsitemap.

joachim’s picture

> So in this case some sort of menu_item_entity module would handle just this entity definition and any other modules that need it require it.

Exactly. Same way that a slim File Entity module was refactored out of Media module: https://www.drupal.org/project/file_entity

mamanerd’s picture

The patch in #15 worked for me as well.

Agileware’s picture

Patch #15 works for me.

  • Dave Reid committed 527e93e on 7.x-2.x
    Issue #2223911 by Grimreaper, justanothermark, Shawn DeArmond | joachim...
Dave Reid’s picture

Status: Reviewed & tested by the community » Fixed

Committed #15 to 7.x-2.x. Thanks everyone.

Status: Fixed » Closed (fixed)

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