Integration with Meta tags API: Facetapi tokens support

Changing status for review.

Thanks in advance.

Quick coding standards update.
Fixed wrong line indentation.
Correct version attached as: facetapi_bonus-metatags-facetapi-tokens-support-1863780-2.patch.

Thanks in advance for your feedback.

Please ignore previous patch from #2.

One more change to test if $options['token types'] is undefined.
This change is also related with #1863050: Some custom contexts may not have token entity mappings.

Submitting updated patch for review: facetapi_bonus-metatags-facetapi-tokens-support-1863780-3.patch

Thanks in advance for your feedbacks, comments and testing.

Alright, thanks to khiminrm's feedback after testing patch from #3 together with #1830952-5: Introduce two new hooks to allow token types and patterns alteration, I found there are pieces of code/logic missing in this patch:

Indeed, this code is greatly inspired from: facetapi_bonus.page_title.inc, as I mentioned initially.
I tested again the page_title integration and it seems the support for some tokens is missing: facetapi_active and facetapi_facet.

I'm not sure whether it's missing intentionally, meaning that as a choice of implementation, these two tokens were left aside for the benefit of the list iterator, which could cover cases for facetapi_active as well. But what about facetapi_facet?

Bringing back to #1856436: Facetapi Tokens: Attempt to initialize tokens based on current active searchers., in particular:

I would assume it would make sense for all the facetapi_tokens replacement values for the results returned for facetapi_results, facetapi_active, and since in a default case facetapi_facet could return information on the last selected facet (same facet as facetapi_active) with different info/properties.

So I've updated the previous patch by simply bringing over the code from #1856436-1: Facetapi Tokens: Attempt to initialize tokens based on current active searchers. to initialize both token values (facetapi_active and facetapi_facet), based on the last selected facet.
[Updated patch attached as: facetapi_bonus-metatags-facetapi-tokens-support-1863780-4.patch]

I would greatly appreciate if I could get your feedback on this implementation choice, as well as a particular line of code, with a regex (line 51):
if (preg_match_all('/[^list<]\[facetapi_active:[^>]/', $pattern, $matches)>0) {
to be able to pick up all occurences of facetapi_active that are not wrapped in a not list<...> iterator (which is another case).

Again, none of all this would be necessary (except code for the list<...> iterator) if we could potentially get the patch from #1856436: Facetapi Tokens: Attempt to initialize tokens based on current active searchers. rolled in facetapi.tokens.inc.

Please let me know if you would have any questions on any points/code/aspects mentioned on this updated patch, 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 these latest changes.
Any feedback, testing, changes, recommendations would be highly appreciated.
Thanks in advance.

Forgot some information about the tested modules versions.

Hi @DamienMcKenna,
Thanks very much for following-up and relating this issue to its corresponding one in Metatag.
 

Hi @Marc Angles,

Thanks a lot for trying to revive this issue and giving this patch a round of testing at #7.
I'm glad to hear it worked for you and would assume it was most likely "crossing your fingers" that made the difference ;)

After almost 2 years, not much has changed in the module and it would seem the patch from #4 would still apply to 7.x-1.x's HEAD.
But in any case, please find attached to this comment a re-roll against facetapi_bonus-7.x-1.x at 1f2e720.
File attached as: facetapi_bonus-metatags-facetapi-tokens-support-1863780-9.patch.

Please let me know if you would have any questions on any points/code/aspects mentioned on this updated patch, 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 these changes.
Any feedback, testing, changes, recommendations would be highly appreciated.
Thanks in advance to all for your testing, reporting and feedback.

Hi guys! :)

Thank a lot to everyone for following up on this and so sorry for dropping out like that from this issue....
Damn, it feels like I posted this ages ago hahaha
(At the time, I was building ecom sites with Commerce and category listing pages powered by Taxonomy/Search API/Facet API/Views and needed a way to automatically fill in/generate metatags for all these listing pages)

I can't begin to imagine the changes in the versions of all the related modules (Search API, Facet API, Meta tags, ....) in those 4 years. But it looks like the patch didn't change that much though.

@thePanz,
Thanks a lot for the re-roll and the review.
I'll try to go over the patch from #19 next week and since you are now the maintainer, do you think there would still be any hope of seeing this committed at some point?

I'll also try writing a bit more information on how to use the feature (or perhaps add some help text/description in README and such), since it seems there was some confusion on how it could be used.

Please let us know if you would have any questions, comments, feedback, issues, objections, suggestions or concerns on any aspects of this patch or this feature request in general, I would be glad to provide more information or explain in more details.

Thanks again very much to everyone for your interest, testing/reporting and feedback.
Cheers!

Hi @thePanz,
Thanks a lot for your nice speedy reply!

Yeah, I think I should be able to get the Page Title code cleared as well.
Let's see what I come up with and if I fall short then I guess we might have to reach to Damien.
But overall, I think I should be able to find my way around without too much trouble.

I'll need a few days to get back to you next week though... I got quite a few things pending already with several other modules I help maintaining.
Very happy to know there is still hope for this feature to make it to the repo :)
Thanks again @thePanz!
More coming soon....

Hi guys,

So sorry for the delay on the follow-up for this issue, but I had a feeling this would be a tough one.... going in many directions, in several different large modules.

Alright, let's dive in straight:

Introduction:

Firstly, I would like to thank @scoff very much for reporting his test results and problems encountered at #25:

Views UI doesn't allow any facetapi tags (they don't appear in the browser).

That's one of the problems I found immediately while performing a simple test.

One thing is for sure, at this level, using such an advanced stack of modules: Views, Search API/Search API Database Search, Facet API, Facet API Bonus and Metatag and configuring them all to work properly together.... we would definitely be talking to confirmed if not advanced site builders and most likely with a programming background.

So I suppose this feature's target audience can probably work its way past most of the common UI/configuration hurdles, pretty much like @scoff did and others tried in this ticket.

Most of the issues I encountered were not "critical" from a programmer's standpoint (if you know what I mean), but damn these edges are rough.... :)
From a site builder's perspective, if not tech savvyy, I would be really amazed if anybody was able to configure these views metatags with tokens.

If I were to configure all these modules for a site myself, it would be super annoying, so I thought I could perhaps try helping polishing up a bit these edges in order to make the configuration process more straight forward and a bit more "site builder friendly".

There is actually very little code to be changed in the current patch and I found most of the small issues are related with the metatag and metatag_views module.

So I'm very happy we got @DamienMcKenna following this issue :D

Problems encountered and corresponding potential solutions:

1 - #2728919: Views Meta Tags popup/overlay/dialog window is too small Patch/solution included
(patches file metatag_views_plugin_display_extender_metatags.inc)
Patches: (pick either)
metatag-views-dialog-too-small-no-empty-markup-2728919-2.patch
metatag-views-dialog-too-small-no-collapse-2728919-2.patch

2 - #2728929: Views Meta Tags Default/Global configuration is ignored Patch/solution included
(patches file metatag_views.module)
Patch: metatag-views-default-config-ignored-2728929-2.patch

3 - #2728933: Facetapi Tokens integration failing in Views UI Patch/solution included
(patches file metatag.module)
Patch: metatag-views-facetapi-tokens-unsupported-2728933-2.patch

4 - Search results (facetapi_results) tokens missing:
This is actually the only problem with the patch in this issue :D
The metatag_views module now defines its own tokens (see metatag_views.tokens.inc, line 10).
Therefore, when the $options array is caught by facetapi_bonus_metatag_token_types_alter to be altered, it already contains an elements, indexed 0.
In the patch from #19, facetapi_results is the first item of the array, thus indexed 0 and therefore gets overwritten by 'view', when the array is merged: $options['token types'] += $types; (see PHP + array operator)

Please find attached to this comment a patch against facetapi_bonus-7.x-1.x at 8630e37, which restores Search results tokens by merging arrays properly.
File attached as: facetapi_bonus-metatags-facetapi-tokens-support-1863780-27.patch.

Just a single line changed:
Replaced: $options['token types'] += $types;
with: $options['token types'] = array_merge($options['token types'], $types);

5 - Improving the documentation: [TODO]To be further discussed:
Consider adding some help texts somewhere for the list<[facetapi_active:facet-label]: [facetapi_active:active-value]> tokens.
This is very specific, I'm really not sure where this could be added, perhaps modifying the README.TXT would be enough? More ideas?
We might maybe need to create a new issue for this.

Results

Once all the corresponding patches (4 patches) have been applied, basically, the configuration process becomes much more logical and straight forward.
1 - Tokens can be configured in the global/default configuration whether overridden or not and views pages resulting meta tags are displayed according to the default settings (including Facet API tokens or not).
2 - Meta tags can be overridden from within a View, with a comfortable popup/dialog window (not the size of 3 lines of text anymore), in which Facet API tokens can be selected through the standard tokens browser: Search results (facetapi_results), Active facet items (facetapi_active) and Facet field (facetapi_facet).
3 - [TODO]Additionally help text/README.TXT would allow users to know the advance usage of token lists: list<XXXX> allowing the display of multiple facets values (which is an essential feature, given the number of facets that can be active on a page).

Example of configuration with which I tested for the description:

[view:description], BLA BLA!!! [facetapi_active:facet-label] | [facetapi_active:facet-name] | [facetapi_active:active-value] | [facetapi_active:active-pos] | [facetapi_active:active-value-raw]
list<[facetapi_active:facet-label]: [facetapi_active:active-value]> [[[[[[ 
[facetapi_results:end-count], [facetapi_results:offset], [facetapi_results:page-limit], [facetapi_results:page-number], [facetapi_results:page-total], [facetapi_results:result-count], [facetapi_results:keys], [facetapi_results:search-path], [facetapi_results:start-count]
]]]]]]]]]]]]
[facetapi_facet:facet-label]: [facetapi_facet:facet-name]
[[[[[[[[[[[[[[[[[[[[[[[[

 
Result:

<meta name="description" content="This is a test description for metatag_views, BLA BLA!!! Weather | field_selectf2 | Sunny | 1 | sun Sports: Fitness, Weather: Sunny [[[[[[ 2, 0, 10, 1, 1, 2, , search-node2, 1 ]]]]]]]]]]]] Weather: field_selectf2 [[[[[[[[[[[[[[[[[[[[[[[[" />

 
Now I understand there are many small changes in multiple modules/files, so I would certainly understand if it took some time to review :) not to mention all the assosciated litterature.
 
In any case, feel free to let me know if you would have any comments, questions, issues, suggestions, objections or ideas on any aspects approached in this comment, any of the patches or related tickets, I would certainly be very happy to provide more information.
Thanks very much in advance for everyone's testing/reporting, reviews, comments and involvement in the development of this feature.
Cheers!

Hi @thePanz,

Thanks for your speedy reply!
Concerning the Page Title integration, I've also given this a bit of thoughts.

To be honest, this may not be the best place to discuss the integration with Page Title (not exactly the object of this issue and I have a feeling our hands are already pretty full)...
Ideally that's something we could discuss in another issue, but I understand you'd like to get @DamienMcKenna's attention and opinion, since custom page titles all go through Metatag now, which Damien is maintaining.

Basically, I assume you would like to know whether we should be removing the code:

// Integration with Page Title module.
module_load_include('inc', 'facetapi_bonus', 'facetapi_bonus.page_title');

from facetapi_bonus.module, see line 10 and corresponding file facetapi_bonus.page_title.inc.
 
Personally (my own opinion), I would be in favor of removing the code, in a specific commit with a different issue (ideally related to the issue for which the page title integration code was committed), telling people the integration with Page Title is deprecated: the Metatag module should be used instead.
 
I mean, the code doesn't do any harm there, but as Damien mentioned at #23:

I honestly wouldn't worry about the Page Title integration, that module is pretty dead.

I would think there wouldn't be any use keeping this code in the module.
 
Let's see what Damien's opinion would be on this.
 
In any case if we wanted to keep the code there, we could perhaps add if (module_exists('page_title')).
 
By the way, just thinking about that now, but do we want to add if (module_exists('metatag')) to the patch #26?
 
Thanks again to everyone for your testing/reporting, reviews, comments and feedback.