I've recently switched to the dev version of Flag (from version 3.2) and now I'm seeing this message everytime I clear the cache:

Notice: Undefined index: token type in flag_token_info() (line 88 of sites/all/modules/contrib/flag/flag.tokens.inc).
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pere Orga’s picture

Investigating the issue, I think it's be a bug of xmlsitemap module, seems to not define a token type.

I'm attaching a patch that checks if there is a token type before trying to add a token. In theory not needed, but IMO these type of checks doesn't hurt.

Pere Orga’s picture

Status: Active » Needs review
joachim’s picture

Doesn't Token module fill in the 'token type' property for all entity types?

Pere Orga’s picture

Sorry, I don't know.

For some reason, entity_get_info() is returning me an entity type menu_link without token type defined, and because the array returned by flag_get_types() contains menu_link I get a notice in some pages.

joachim’s picture

It's http://drupalcode.org/project/token.git/blob/refs/heads/7.x-1.x:/token.m... but I haven't time right now to read the function and figure it out...

joachim’s picture

Status: Needs review » Postponed (maintainer needs more info)

AFAICT, the token type is filled in for any entity type that doesn't define it:

 442 // By default the token type is the same as the entity type.
443 $info[$entity_type]['token type'] = $entity_type;

Is xmlsitemap module doing something weird to prevent this?

mparker17’s picture

Status: Postponed (maintainer needs more info) » Active

I get this error on every page request on my site, including page requests to the backend of the site (running the Seven theme, not my custom theme).

I'm running Drupal 7.26 on both Pantheon (PHP 5.3.17) and my local machine (PHP 5.4.10). I'm running flag-7.x-3.4 (flag_actions and flag_bookmark are disabled), token-7.x-1.5 and xmlsitemap-7.x-2.0.

It looks like the superficial cause of the error is that $entity_info['menu_links']['token type'] doesn't exist. It does exist for all the other things in the $entity_info array. The $entity_info array is retrieved in the previous line of code by calling the core function entity_get_info().

After a bit of debugging, it looks like the function xmlsitemap_menu_entity_info_alter() in xmlsitemap_menu/xmlsitemap_menu.module adds the menu_links sub-array, but does not provide a token type key.

Looking at the current D7 documentation for hook_entity_info() and hook_entity_info_alter(), it appears that the token type key is not required (in fact, it's not even mentioned in the API specification).

This omission could be an oversight in the core documentation, but regardless: if it's not in the spec at the current time, then we can't expect other currently-existing contributed modules to know about it and provide it. I think @Pere Orga's patch is the correct solution to this problem.

mparker17’s picture

Status: Active » Reviewed & tested by the community

I can confirm that @Pere Orga's patch meets Drupal coding standards and applying the patch fixes it on my site.

As far as I'm concerned, this patch is RTBC.

joachim’s picture

Status: Reviewed & tested by the community » Postponed

The right fix is over here: #2223911: xmlsitemap_menu.module uses hook_entity_info_alter(), and so doesn't get a token type set by token module.

It's always better to try to fix the root cause of a problem than to hack around it.

xbrianx’s picture

I am also getting this issue after updating. This patch worked for me.

DamienMcKenna’s picture

Status: Postponed » Reviewed & tested by the community

The 'token type' value is provided by the Token module, it isn't part of core's token system. XMLSitemap doesn't list the Token module as a dependency, therefore it shouldn't be assumed to work with its APIs. Also, Flag doesn't list Token as a dependency, though is using an API change that only Token provides. I recommend applying this fix and updating the module's dependencies list to include Token.

joachim’s picture

Status: Reviewed & tested by the community » Postponed

Token is not a dependency, it's just a module that we support if it's there.

DamienMcKenna’s picture

Ok, I missed the "if(module_exists('token'))" part, sorry for misreading the code last night.

Upon mature reflection =) I concur that the problem is not Flag's fault. That said, the patch above does solve the problem.

Pere Orga’s picture

Hi

The patch shuts up the notice but does it solve the problem? Or should the issue be fixed in #2223911?

What I see from grep -r "token type" * is that all other contrib modules do check if 'token type' is set, before trying to access it.

funtik44’s picture

I have this problem too after update to version 3.5.

marcoka’s picture

problem confirmed here too.

darol100’s picture

Status: Postponed » Needs review
joachim’s picture

Status: Needs review » Postponed

See comment #9.

Shawn DeArmond’s picture

I submitted a patch to #2223911: xmlsitemap_menu.module uses hook_entity_info_alter(), and so doesn't get a token type set by token module

Please try it out and see if it works, but more importantly, if it breaks anything else.

brice_gato’s picture

It will be well to anticipate this case which costs nothing, because in the future it may come from another module other than XMLSitemap.

thedavidmeister’s picture

Status: Postponed » Reviewed & tested by the community

It's always better to try to fix the root cause of a problem than to hack around it.

I disagree that what is being suggested here is a hack.

From the PHP manual:

Relying on the default value of an uninitialized variable is problematic in the case of including one file into another which uses the same variable name. It is also a major security risk with register_globals turned on. E_NOTICE level error is issued in case of working with uninitialized variables, however not in the case of appending elements to the uninitialized array. isset() language construct can be used to detect if a variable has been already initialized.

What we are doing here is relying on the default value of an uninitialized variable. Whether we think the variable "should" be initialized or not somewhere else, if we're not initializing it ourselves, checking that it has been initialized before trying to access it is the correct approach, as per the PHP documentation.

joachim’s picture

Status: Reviewed & tested by the community » Postponed

> Relying on the default value of an uninitialized variable is problematic in the case of including one file into another which uses the same variable name.

That's about global variables.

rooby’s picture

I wouldn't necessarily say the solution in the patch is a hack, however I would classify it as a band-aid solution.

It is masking the real error instead of fixing the root cause, which IMO is practically always a bad idea.

stefan.r’s picture

kscheirer’s picture

I don't have xmlsitemap installed, so my error is
Notice: Undefined index: comment in flag_token_info() (line 72 of /path/flag/flag.tokens.inc).
This is while using Drupal Commons 7.x-3.15 which ships with Flag 7.x-2.2

The patch in #1 definitely suppresses the error. I get that it's not the best fix, but if more modules are getting this wrong some defensive programming is in order. You could add a watchdog notice about the problem instead of just skipping over it though.

joachim’s picture

> Notice: Undefined index: comment

That's a different error message -- please file a new bug.

james.williams’s picture

Status: Postponed » Reviewed & tested by the community

This notice can occur for any flag type that isn't for an entity. The documentation for hook_flag_type_info() says that 'When a flag type is for an entity, the flag type name must match the entity type' but not that flag types have to be for entity types. If a flag type exists that is not for an entity type, then the line that is throwing this notice is making a false assumption - that there will be entity info set for the flag type.

This line of reasoning suggests that either the patch in comment 1 on this issue, or the one at #2295127-3: Undefined index: token type in flag_token_info() ligne 88 in flag/flag.tokens.inc, should be entirely appropriate. Yes, it's good to fix the issue(s) at source, and whilst this thread has helped identify one in xmlsitemap (and potentially comment module, in kscheirer's situation), it's just not correct to assume that the token type or entity info must have been defined for a flag type. The only effect that applying either of the patches would have is to stop those tokens from appearing as available ... which may itself still indicate the underlying problem if the flag is for a valid entity type. This means that even without the notice, there is still indication of an underlying problem if the user expects there to be tokens available for a flag - so the patch to get rid of the notice fixes the issue (when valid to do so, as explained above) without removing all indication that there may be an underlying problem (i.e. if the flag is for an entity type).

Given that, I'm updating this ticket to RTBC again.

james.williams’s picture

I should point out that if the module maintainer disagrees, then the flag type documentation must be updated to explicitly indicate that flag types must all be for a specific entity type, which I don't believe needs to be the case - Flag provides a very useful API, and I see no reason for it to force the keys of flag types to all match entity types. Perhaps if anything, there could be a way to define what entity type a flag type is 'for' (so that tokens etc could use this) that does not have to be the key of the flag type.

joachim’s picture

Status: Reviewed & tested by the community » Needs work

> but not that flag types have to be for entity types

Oh well spotted!

However, the patch needs work then -- it should merely be checking that the flag type is an entity type.

Also, rather than an if block I'd rather have an inverted condition with a bail at the top of the loop.

PlayfulWolf’s picture

rlmumford’s picture

Status: Needs work » Needs review
FileSize
605 bytes

Updated patch that checks whether the flag type is an entity type and skips to the next iteration if not. Getting this committed would be awesome, and all the watchdog calls introduced by this error are seriously slowing our sites down.

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

patch looks good

joachim’s picture

Thanks!

I'd prefer an !isset() for clarity, but unless someone rerolls by the time I get round to committing this, it can go as it stands ;)

stefan.r’s picture

FileSize
606 bytes

  • joachim committed 9efa556 on 7.x-3.x authored by Pere Orga
    Issue #2197041 by Pere Orga, rlmumford, stefan.r: Fixed PHP notice in...
joachim’s picture

Status: Reviewed & tested by the community » Fixed

Committed.

Thanks everyone!

For those who are particularly keen to see this in a new release, there is a list of issues that are tagged as blockers for 7.x-3.7: https://www.drupal.org/project/issues/search?issue_tags=7.x-3.7%20blocker /shameless plug ;)

Status: Fixed » Closed (fixed)

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

Road Kill’s picture

I am not but it looks like this is still a problem

Notice: Undefined index: token type in flag_token_info() (line 93