Hi guys,

Thanks very much for the great work and it seems there has been quite a lot going on lately, at the look of the latest commits.

Working with metatag [7.x-1.0-beta2+0-dev (2012-10-30)], metatag_views [sandbox], I've been trying to configure patterns along with the search_api/search_api_views and facetapi module.

With this setup I get views pages provided by search_api_views along with facetapi blocks which enables navigation/filtering.

While trying to optimize the page titles of this particular type of views pages, for example to enable dynamic generation of the page titles based on the filtered facets, I investigated further facetapi, search_api, and it seems these modules define various tokens such as: Current active facets or Search results count (mainly through facetapi).

As a result, I found Page titles could be dynamically generated by using the modules page_title and facetapi_bonus.
The main reason for that seems to be that page_title provides a specific hook for other modules to extend token types and alter patterns/replacements.
page_title, latest 7.x-2.x-dev, page_title.module, line 606:
drupal_alter('page_title_pattern', $page_title_pattern, $types);

this is in turn implemented in:
facetapi_bonus, latest 7.x-1.x-dev, facetapi_bonus.page_title.inc, line 11:
facetapi_bonus_page_title_pattern_alter(&$pattern, &$types)

Therefore, facetapi_bonus is able to expose to page_title the different tokent types and patterns.

Which brings this feature request for the metatag module, to be able to expose token types and alter patterns.
Would it be possible to extend this module along the same lines as the page_title module?

Would it be possible to add the two following hooks to allow other modules to extend defined/supported tokens?

Looking at the code of the metatag module, 7.x-1.0-beta2+0-dev (2012-10-30):

1 - hook_metatag_token_types_alter
metatag.admin.inc, line 276
$options['token types'] = array(token_get_entity_mapping('entity', $contexts[0]));

metatag.module, line 992
$options['token types'] = array(token_get_entity_mapping('entity', $entity_type));

Currently, it seems supported token types are limited to entities.
Would it be possible to allow other modules to alter token types?
drupal_alter('metatag_token_types', $options['token types']);

to be further implemented as an integration in facetapi_bonus, for example:

function facetapi_bonus_metatag_token_types_alter(&$token_types) {
    $types = array('facetapi_results','facetapi_active','facetapi_facet');
    $token_types = array_merge($types, $token_types);
}

This allows to be able to expose these tokens in the admin forms (fields/pages).

2 - hook_metatag_pattern_alter
metatag.inc, line 107
$value = token_replace($value, $options['token data'], $options);

This is pretty much where data is being replaced.
Could we potentially get a chance to alter patterns/replacements before they actually get replaced?
drupal_alter('metatag_pattern', $value, $options['token data']);

which would allow us to further implement its hook in facetapi_bonus, for example:

function facetapi_bonus_metatag_pattern_alter(&$pattern, &$types)
//This is where we do the replacement work.
// If we meet list<...[facetapi_active:XXX]...> string we replace it with
// coma separated list of tokenized string.
// So list<[facetapi_active:facet-label]: [facetapi_active:active-value]>
// will be replaced with "type: node, author: admin" string if type and
// author facets active.

Please find attached to this ticket a suggested patch against the latest dev (very short: 3 lines), on which I would really be glad to get your feedback and comments.
[Attached file: metatag-introduce-hooks-token-types-patterns-alter-0.patch]

It is based on the explanation above, has been tested and indeed, it seems to work fine with the use case described: facetapi, search_api, facetapi_bonus.
If you found this patch and these hooks of use, I would be glad to help updating the patch with its corresponding documentation in the metatag.api.php file.

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 doable already with existing hooks?
But for this particular use case, I haven't been able to find any appropriate existing solution.

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 take a look at the patch and give me your feedback on this feature request.

Feel free to let me know if you would have any questions about the code or any other aspect of the patch, I would be glad to explain in more details.

Thanks in advance.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DYdave’s picture

Status: Active » Needs review

Changing status for review.

DYdave’s picture

Issue summary: View changes

Edited sandbox link

DamienMcKenna’s picture

Status: Needs review » Needs work

Sounds like a reasonable idea, especially if there's already a good use case for it (customizing meta tags for search results display). If you'd be so kind as to provide some documentation I'll be happy to test this further. Thanks.

DYdave’s picture

Status: Needs work » Needs review
FileSize
5.07 KB

Hi Damien,

Thanks very much for your prompt reply and I sincerely apologize for the delay of my answer, but it took me a while to do some more testing, research and get back to you with a serious follow up.

I have slightly updated the patch and added documentation to the file metatag.api.php as you kindly invited me to do so.
Please find attached the updated version of the patch against metatag [7.x-1.0-beta4+20-dev (2012-12-10)]:
metatag-introduce-hooks-token-types-patterns-alter-1830952-3.patch

So back to our two new hooks:

1 - hook_metatag_token_types_alter
The biggest change here, from previous patch is that before it was passing $options['token types'] to hook_metatag_token_types_alter (see initial patch from post) and I thought we might as well want to pass the full $options array in order to get the context property/key in the hook function for testing on the context, for example.
This change led to moving the call to hook_metatag_token_types_alter in metatag.module down one line, right after: $options['context'] = $entity_type;, to be able to get the context property/key initialized before calling the hook.

I would also like to bring to your attention on the fact that if $options['token types'] is not initialized, as suggested by Dave Reid's patch for #1863050: Some custom contexts may not have token entity mappings, then $options['token types'] has to be tested in the implementation of hook_metatag_token_types_alter before adding values to the array, merging, array pushing or concatenating in any way.
I've tried to quickly explain that in file metatag.api.php for the implementation of hook_metatag_token_types_alter:

    // Watchout: $options['token types'] might be empty
    if (!isset($options['token types'])) {
      $options['token types'] = array();
    }

Another solution would be to initialize with an empty array before calling the hook, and that's why I came up with the proposed patch at: #1863050: Some custom contexts may not have token entity mappings.
So this is still under discussion and I would really appreciate to have Dave Reid's feedback on this as well.


Perhaps this part could be optimized, so that's why I preferred to be straight and let all of you guys know about it.
I would greatly appreciate your feedback on this, if it could be simplified or optimized in any way.

 

2 - hook_metatag_pattern_alter
The hook_metatag_pattern_alter is the same as in the previous patch and I've added corresponding documentation to the file metatag.api.php.
I have documented some short examples of possible implementations of the hooks as well, for initialization of $data before replacements or patterns alteration.

As a side note here, I just wanted to mention that if the modules Facet API in facetapi.tokens.inc, or Facetapi Bonus could implement some kind of facetapi_active_list token for a list of all active facets, alteration of the pattern wouldn't be necessary in facetapi_bonus.metatag.php. That's another feature request that I will bring elsewhere, but I just wanted to document a few line here as well.

 

I would assume these hooks could also be used to integrate with Views tokens, but so far I have only been mostly testing all this with Facetapi tokens.
I would surely be very happy to hear others' experiences integrating this with Views tokens or other modules implementing tokens.

For improved testing and validation of this patch, I am going to follow up shortly with the corresponding patch for integration with Facetapi Bonus to provide support for Facet API tokens in metatag patterns.
This way, everyone will also be able to look into the consistency of the hooks, documentation and real use case implementation examples (To be followed up shortly in current issue).

Could you please let me know if I overlooked or missed anything in the current metatag module (and its API)?
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.

DYdave’s picture

Status: Needs review » Needs work
FileSize
5.07 KB

Please ignore patch from #3.

Indeed, the tests managed picking up a minor syntax error in metatag.api.php.
Submitting updated patch for automated testing again.
metatag-introduce-hooks-token-types-patterns-alter-1830952-5.patch

Thanks in advance for your feedbacks, comments and testing.

DYdave’s picture

Status: Needs work » Needs review

Changing status for review.

DYdave’s picture

Hi guys,

Just a quick follow up to let you know the patch for Facetapi Bonus [facetapi_bonus-7.x-1.1+7-dev (2012-10-22)] for #1863780: Integration with Meta tags API: Facetapi tokens support has been posted in #2.

For testing, simply:

  1. Apply patch from #5 to metatag-7.x-1.0-beta4+20-dev (2012-12-10)
  2. Apply patch from #1863050: Some custom contexts may not have token entity mappings, #3 to metatag-7.x-1.0-beta4+20-dev (2012-12-10)
  3. Apply patch from #1863780: Integration with Meta tags API: Facetapi tokens support, #3 to facetapi_bonus-7.x-1.1+7-dev (2012-10-22)

I have tested with Views, Metatag tags, Meta tags views (ribakker), Search API, Facet API, Facet API Bonus, these are probably the most concerned by these patchs and tests.

Patterns examples:
You should then be able to configure your patterns for Search Api/Facet API enabled Views pages and customize further the token patterns in the Meta tags administration pages, for example:
Meta Description:
[facetapi_results:result-count], [facetapi_active:facet-label], [facetapi_active:active-value], [facetapi_facet:facet-name]

In the list of available tokens, on the configuration page, all the tokens introduced by Facet API should be visible, such as Search results, Active facet items, Facet field.

Feel free to let me know if you would have any questions, comments or particular issues on the testing of any of the patches or described items, I would surely be glad to provide more details.

Once again, any feedback, comments or suggestions would be highly appreciated.
Thanks to everyone in advance.

khiminrm’s picture

Hi! I've tried to apply patches to latest dev version of Meta tags and got error on patch 2 "Hunk #1 faild at 273.
Then I've manualy edit the file eccording to the patch.
And tried to add the token [facetapi_active:facet-label] in admin page of metatags (By Path and for the view with Meta tags views (ribakker)). And got error message that the token is wrong. How to deal with latest dev of Meta tags?

khiminrm’s picture

After clearing the cache I can add [facetapi_active:facet-label] in title for the view meta tags and save, but it doesn't work for me.
In meta tag by path still have error message.

DYdave’s picture

Hi khiminrm,

Thanks a lot for your feedback and taking the time to test the patches from #7.
It is certainly highly appreciated.

Let me try following up with your comments:

I've tried to apply patches to latest dev version of Meta tags and got error on patch 2 "Hunk #1 faild at 273.

Yes, indeed, I guess there's going to be a problem with the line number to apply the patches. Thanks again, for taking the time to manually apply 2 #1863050-3: Some custom contexts may not have token entity mappings. I'm not sure exactly how to deal with that from a patching standpoint.
Perhaps I should create this current patch after having applied 2 first, GIT commit in my local branch, then create this patch again, so the line number would be correct.

So just for testing purpose, I have combined updated/attached patch:
metatag-introduce-hooks-token-types-patterns-alter-1830952-10.patch with #1863050-3: Some custom contexts may not have token entity mappings, which should bring the testing process to the application of only two patches (this one and the one from facetapi_bonus, see below).
Combined patch attached as: metatag-introduce-hooks-token-combined-1863050-3-testing-1830952-10.patch

In any case, with the two patches combined (from #1863050: Some custom contexts may not have token entity mappings and the one attached to this comment) the line 276 from metatag.admin.inc:

$options['token types'] = array(token_get_entity_mapping('entity', $contexts[0]));

should be replaced with:

    if ($token_type = token_get_entity_mapping('entity', $contexts[0])) {
      $options['token types'] = array($token_type);
    }
    // Allow hook_metatag_token_types_alter() to modify the defined tokens.
    drupal_alter('metatag_token_types', $options);

I seized this opportunity to update the patch from #5 against the latest GIT version, see attached:
metatag-introduce-hooks-token-types-patterns-alter-1830952-10.patch

 

Concerning your next comment:

After clearing the cache I can add [facetapi_active:facet-label] in title for the view meta tags and save, but it doesn't work for me.

I think this was due to missing tokens integration, please take a look at #1863780-4: Integration with Meta tags API: Facetapi tokens support, I have already updated the patch for facetapi_bonus.

 

Lastly, for:

In meta tag by path still have error message.

I'm very curious here what you would mean exactly by meta tag by path still have error message:
Could you please provide a path where this error would happen?
Could you please provide the error message that is prompted or perhaps the information from the log reporting?
Ideally, I wish I could reproduce this issue to be able to fix it.

Any additional information that would allow further debugging would be highly appreciated.

 

One more very important remark: (That isn't documented anywhere, except in code)
Since the integration of Meta Tags with Facetapi is based on code from facetapi.page_title.inc, a list<...> token iterator is supported and defined on facetapi_active token.
Feel free to test with the following pattern:

[facetapi_results:result-count], [facetapi_active:facet-label], [facetapi_active:active-value], list<[facetapi_active:active-value]>, list<[facetapi_active:active-pos]> |||| [facetapi_facet:facet-label], [facetapi_facet:facet-name] ||| list<[facetapi_active:active-value-raw]>, list<[facetapi_active:facet-name]>

which is more complete than the previous from #7.

Please let me know if you would have any questions on any points/code/aspects mentioned in my answers, I would surely be glad to provide more information.

I would greatly appreciate if you could take a bit more time to take a look again at these updated patches and give me your feedbacks.
Thanks to all in advance.

khiminrm’s picture

Hello, DYdave! Thanks for your quick reply and modifications to the patches. I've just tested your updated patches: metatag-introduce-hooks-token-combined-1863050-3-testing-1830952-10.patch and Updated patch to add support for facetapi_active and facetapi_facet tokens. All work fine! I've also tested all tokens from #10. You have done a great work! For testing I used Meta tags views (ribakker) for my Search API view. In my previous comments I wrote about meta tags by path. I mean the tab "BY PATH" in meta tags administration page when Meta tags: Context is enabled. But even with newer patches the tokens of facet api can't be used when adding a new meta tag by path. For example when I use list<[facetapi_active:active-value]> in the Title I got the error when saving the page: "The Page title is using the following invalid tokens: [facetapi_active:active-value]".
Thanks again!

khiminrm’s picture

Hello, again. I have found that [facetapi_active:facet-label] and list<[facetapi_active:facet-label]> contain also machine name of facet field after Label of facet and "-". I think it would be better if [facetapi_active:facet-label] will not contain machine name of the facet field. And one more question: Is it real to do a token that will render such list: Active facet 1 label: list of active values for facet 1, Active facet 2 label: list of active values for facet 2 and so on? Thanks!

khiminrm’s picture

Hello, DYdave! in " I have found that [facetapi_active:facet-label] and list<[facetapi_active:facet-label]> contain also machine name of facet field after Label of facet and "-". " Sorry, it's my mistake. The facets were overriden in my custom module.

Dave Reid’s picture

I'm having a hard time following and understanding why these changes are necessary. I may need to give this more time to review this weekend when I have a bit more focus.

DYdave’s picture

@khiminrm,

Sorry for this late reply, but I wanted to thank you very much for your time on this, applying all the different patches and giving us your feedback on the testing.
I'm very happy you were able to get this to work out in the end and found out what was causing a conflict with the facet tokens.

@Dave Reid,

Thank you so much for getting back to me on this. I would greatly appreciate your feedback and review on this feature request, when you have some time. Perhaps you would be able to suggest other ways this could be achieved, without changing the code and I would surely be very grateful to hear your recommendations.

Please let me know if you would need any more information, I would certainly be glad to explain in more details.

Thanks again to all for your time, testing, feedbacks and comments.

havran’s picture

Hi, any news about this issue? Thanks for all works.

DamienMcKenna’s picture

Should this not be possible using normal token hooks, e.g hook_tokens_alter?

DamienMcKenna’s picture

Status: Needs review » Postponed (maintainer needs more info)
DYdave’s picture

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

@DamienMcKenna:
Thanks a lot for your reply and suggestion, it's greatly appreciated.

So let's go back to the start:
(The idea/use case here is to provide integration with Facetapi/Search API tokens)

An initial problem would be to make Facetapi/Search tokens available so that the patterns could be saved in the instance config edit form (currently these tokens are not available in these forms, see #11 for more information on what happens when the token types are not defined).
I assume they would need to be declared in metatag_config_edit_form in metatag.admin.inc, line 263, where the token types currently seem to come from $contexts[0], initialized at $contexts = explode(':', $config->instance);, so in this case it would basically contain 'views' (searchapi views page, see issue summary metatag_views).

The problem is that for this particular use case: facetapi_results, facetapi_active, facetapi_facet:
(a suggested implementation of the hook_metatag_token_types_alter, see patch at #1863780-4: Integration with Meta tags API: Facetapi tokens support)

<?php
function facetapi_bonus_metatag_token_types_alter(&$options) {
  if (isset($options['context']) && $options['context'] == 'views') {
    $types = array(
      'facetapi_results',
      'facetapi_active',
      'facetapi_facet',
    );
    if (!isset($options['token types'])) {
      $options['token types'] = array();
    }
    $options['token types'] += $types;
  }
}
?>

to support these types of patterns, we would need to potentially be able to add multiple token types to be supported in a single pattern.
As suggested in #10:

[facetapi_results:result-count], [facetapi_active:facet-label], [facetapi_active:active-value], list<[facetapi_active:active-value]>, list<[facetapi_active:active-pos]> |||| [facetapi_facet:facet-label], [facetapi_facet:facet-name] ||| list<[facetapi_active:active-value-raw]>, list<[facetapi_active:facet-name]>

If we could already save these types of patterns in the system, it would perhaps allow further discussion related to the integration with Facet Api and Search API token patterns or types, through Facet Api Bonus (see also #1863780: Integration with Meta tags API: Facetapi tokens support).
(So replacements or tokens alteration is not really in question yet)

I would certainly greatly appreciate if you could perhaps suggest a better way this could be achieved, or if I overlooked or missed anything in the current metatag module (and its API), in terms of whether this would be doable already with existing hooks?
But for this particular use case, I wasn't able to find any appropriate existing solution.

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 take a look at the patch and give us your feedback on this feature request.

Feel free to let me know if you would have any questions about the code or any other aspect of the patch, I would be glad to explain in more details or re-roll the patch if you could consider this feature request as reasonable/acceptable.

Thanks in advance to all for your comments, reviews, testing and reporting.
Cheers!

DamienMcKenna’s picture

Could you please explain why this could not be done by just creating some additional global tokens?

zterry95’s picture

Hi DamienMcKenna

We are actually using the patch written by DYDave in our site, and it works well.

While trying to answer your question :

Could you please explain why this could not be done by just creating some additional global tokens?

I have spent some time to look into the code. Actually, from below line:
http://drupalcode.org/project/metatag.git/blob/HEAD:/metatag.admin.inc#l273

The token type was introduced. But we can't change the token types without any drupal hooks...

So I would think DYDave's patch is reasonable.

Glad to hear your comments:)

DamienMcKenna’s picture

Status: Needs review » Needs work

Ok, I read through the FacetAPI code and now see why this is needed - you're trying to leverage the tokens that FacetAPI itself provides. If you can please reroll the patch I'll be happy to include it in the next beta release.

cm_is’s picture

Hi,

any update on this on? Also very interested in generating meeningfull meta descriptions for search api views. Also a integration for meta robots tag, as well as the canonical tag would be very useful in terms of onpage seo optimisation. I would be more than happy to provide input from a seo perspective.

Greetings cm

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
3.59 KB

It seems that I accidentally committed the change to metatag.inc earlier this year. Doh. This is a reroll of the remaining parts.

DamienMcKenna’s picture

Status: Needs review » Fixed

Committed.

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

Anonymous’s picture

Issue summary: View changes

forgot attached file name

DamienMcKenna’s picture

Issue tags: -metatag