Hi guys,
Thanks very much for the great work on the API and following up with the bug fixing.
I just wanted to report a minor issue I encountered while working with the latest versions of the metatag [7.x-1.0-beta4+19-dev (2012-12-06)] and metatag_views [sandbox] modules.
This is also related with #1830952: Introduce two new hooks to allow token types and patterns alteration somehow, since it's related with tokens and token types.
In file metatag.admin.inc, line 276, function metatag_config_edit_form:
if ($contexts[0] != 'global') {
$options['token types'] = array(token_get_entity_mapping('entity', $contexts[0]));
}
So in this particular use case with the metatag_views module, which introduces a views context, is not global and has no entity mappings, the call token_get_entity_mapping('entity', 'views');
returns FALSE.
As a result, $options['token types'] = array(0 =>FALSE);
is passed to metatag_metatags_form($form, $config->instance, $config->config, $options);
Through the $options
parameter.
So I thought perhaps we would want to avoid adding any value to the $options['token types']
array if the function token_get_entity_mapping returned FALSE.
In general, I would assume this could happen not only with the metatag_views (ribakker) (that I have tested), views_metatags (Dave Reid), but I would assume any module that implements a context/config that isn't an entity mapped with tokens or global.
This would matter as well for: #1830952: Introduce two new hooks to allow token types and patterns alteration since, it's introducing a hook to alter token types (see: drupal_alter('metatag_token_types', $options['token types']);
) that would have a FALSE value in it instead of being empty, for example.
Please find attached to this ticket a patch that attempts to resolve this issue:
metatag-custom-contexts-not-have-token-mappings-0.patch
I tried to add few words in a comment to explain this aspect of the logic.
Could you please let me know if I overlooked or missed anything in the current metatag module (and its API), in terms of whether this would be a problem or not?
Please let me know if you would have any questions on any points/code/aspects mentioned in this ticket, I would surely be glad to provide more information.
I would greatly appreciate if you could take a bit of time to look into the patch and give me your feedback/opinion on this issue.
Any feedback, testing, changes, recommendations would be highly appreciated.
Thanks in advance.
Comment | File | Size | Author |
---|---|---|---|
#3 | metatag-custom-contexts-not-have-token-mappings-1863050-3.patch | 726 bytes | DYdave |
metatag-custom-contexts-not-have-token-mappings-0.patch | 889 bytes | DYdave | |
Comments
Comment #1
DYdave CreditAttribution: DYdave commentedChanging status for review.
Thanks very much in advance.
Comment #2
Dave ReidI would probably prefer a much cleaner version:
Would you be able to re-roll with this change?
Comment #3
DYdave CreditAttribution: DYdave commentedHi Dave,
Thank you very much for your prompt answer, taking the time to go through this detailed post and looking at the patch.
I certainly greatly appreciate your feedback: it can only improve the code of this great module.
Please find attached the updated version of the patch against metatag [7.x-1.0-beta4+20-dev (2012-12-10)]:
metatag-custom-contexts-not-have-token-mappings-1863050-3.patch
One major concern about this update though:
In this updated version of the patch,
$options['token types']
wouldn't be initializedwhich would have to be tested in implementations of hook_metatag_token_types_alter.
Therefore, I would like to kindly invite you to take a look at: #1830952: Introduce two new hooks to allow token types and patterns alteration and more particularly the patch submitted at #3.
The part that says:
Could you please let me know if I overlooked or missed anything in the hooks implementations and concern related with current issue?
I would greatly appreciate if you could take a bit of time to take a look at this updated patch and give me your feedback.
Feel free to let me know if you would have any questions about the code or any other aspect of the patch or ticket in general, I would be glad to explain in more details.
Thanks very much in advance.
Comment #4
Dave ReidThis was solved in a duplicate issue: #1773926: Token validaiton fails on config edit if the instance context isn't an entity type so marking this as a duplicate of that one.
Comment #5
DamienMcKenna