Make Metatag work with Views, ala the 7.x-1.x branch.

Files: 

Comments

DamienMcKenna created an issue. See original summary.

DamienMcKenna’s picture

The D7 version uses a display_extender plugin, need to investigate the best approach to either porting it or to find out what the D8 equivalent is.

Michelle’s picture

delta’s picture

Assigned: Unassigned » delta
Status: Active » Needs work
FileSize
5.77 KB

Hi, I attached a really straightforward port of the display extender plugin.
I would need some hint about the way to attach the metatag form to the extender plugin, the D7 version use an helper function metatag_metatags_form($form, $instance, $metatags[LANGUAGE_NONE], $options);

After that the work to enable the page meta is done in metatag_views_page_alter() in D7, need to see if it's the way to use in D8 too, some advices ?.

For now the patch just enable properly the metatag_display_extender views plugin.

I will look at that later, I'm really want to work on that, some help would be welcome to understand the new d8 metatag API.

Also, I think we need to add default metatag for views in general (like for node), need a look at that one too.

Thanks

delta’s picture

FileSize
2.37 KB

I did this little module called "page_title_views" and that use the display extender plugin to set the page title of a views page. I hope this will help to see how things can be done from now :)

Edit: This is the version that works with the cache, but that use the route to discover the views and get the display extender settings in the preprocess html.

delta’s picture

I also open this issue in views because I don't understand why I can't retrieve a views page in a hook_preprocess_html with views_get_current_view().

https://www.drupal.org/node/2665170

delta’s picture

Ok I got some help from @dawehner on the views issue, it was because of the cache, using the hook_views_pre_render() and adding my title to #attached [html_head] fix the issue.

However in addition of metatag module, that's not working anymore, because for a reason I didn't explain you rewrite the html_head based on the content of a static variable that is written by the metatag_manager service.

When I use this static to populate my tag in the hook_views_pre_render(), that works only after a cache rebuild, after the title is not present anymore (cause of the cache hook_views_pre_render() is called once after the static is not populated anymore).

Why do you use a static to rewrite the html_head, using hook_page_attachments and hook_preprocess_html ? and why are not using the #attached [html_head] the way is supposed to be used ?

I think this make the views integration this way quasi impossible ... or do you see another way to solve this ?

Also now the metatag form seems to be based on fields, so I have no way to attach it to the views in straightforward way, I would need to make a fieldable entity attached to the views config, that seems a lot to just have some metatag on a views. I saw the metatagManager->form() method but I doesn't see how to use it..

So I'm surely motived to help you on that, but it seems there is some choice has been make and you need to tell me what you have in mind for the views integration when you make it, and that before releasing a 1.1 because it's maybe now the time to think about that (it's again time to break the code :)).

DamienMcKenna’s picture

@delta: Thanks for all the work you've done on this! I'm most likely going to bump this up to beta6, just because Views is in core so Metatag needs to work with it, and like you suggest, there may be some API changes needed in order for it to work.

delta’s picture

This is the version that has a bug with the cache that use hook_views_pre_render() and the $metatag static variable.

delta’s picture

Good to hear ! I will do a break for now so that's let you think of this, but if you need to talk about all of that things you can message me when you have unmixed all that things in your mind :)

delta’s picture

I think this is pretty bad : https://www.drupal.org/node/2665378

And from what I understand is maybe why you use a static to retrieve tags values in hook_preprocess_html...
cause you forced to do things inside the hook_preprocess_html().. But the #attached [html_head] normally let you do that within the context of your choice..

tom_ek’s picture

Hi, here`s a patch that implements the very basics of the module. This is mostly based on @delta work so far plus a few modifications in a metatag module itself (and metatag manager).

Since the views module is in core I think that it makes sense to add the route discovery logics to metatag core module. Otherwise we would need to reimplement the same hook (or use alter) in metatag_views module which would cause running more less the same code twice. Maybe it`s a good place to use some kind of service? Or auto discoverable plugins for route - entity matching? That`s to be discussed.

As I said - this is the very beginning of the implementation. There`s no defaults. The patch just adds the possibility to add metatags for each view and displays them.

Comments most welcome.

DamienMcKenna’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

Assigned: delta » Unassigned

Given that Views is in core, maybe we should move the Views integration into Metatag itself too, rather than a submodule?

tom_ek’s picture

Views are in core, but they can be not installed (although I highly doubt that is an often case ;) ). In this case this adds a little bit of overhead on each page (checking the path if it is a view page). On the other hand what we have now has to check if a module exists on each page.

Now the other thing is - what about the entity detection? At the moment this is pretty much hardcoded - checking whether a page is an entity or a view. Maybe we could implement some kind of plugin? That would take care of checking the route, checking it`s default values and checking the specific entity values? Than we could just call each of the plugins, and if it`s route matcher says that this plugin applies we could run methods for getting the defaults and getting the specific metatags. Does that make sense? Will there ever be more than just ContentEntities and Views?

DamienMcKenna’s picture

I'm all ears on suggestions!

tom_ek’s picture

Okay, so I kept on working on this matter. So far I have kept the modules separated as that is easier to maintain and allow to not reroll the patches every time something small is updated :).

Basically the main concept stayed the same - ViewsDisplayExtender with a few improvements.

But there is more - so far the only way to edit metatags for views was to actually edit a view. Having in mind that most of the time the SEO stuff is not done by drupal admins. this might not be a very good idea to give them access to edit views screen. Because of that I have implemented the "central" UI to manage the views metatags. I have placed it as a tab on metatag defaults edit page. The whole UI is based on the global metatags UI.

Would be great if someone had a minute to look at it and give some feedback :). Next step is to implement per view defaults (do you think it is required?).

DamienMcKenna’s picture

@tom_ek: There was a separate request somewhere to allow users able to do that without going into the Views UI (ditto for Panels). With your UX can the Metatag configurations still be edited within the Views UX or is it all done in the separate interface?

Status: Needs review » Needs work

The last submitted patch, 17: metatag-port_views_integration-2563647-17.patch, failed testing.

tom_ek’s picture

@Damien - editing via Views UI is still possible. Basically I have left what was there and added a separate UI that allows you to modify the views display extender settings on a separate form. So - nothing was removed. Just additional functionality have been built upon what was done so far. I have added an option to "Revert" the configuration to fall back for globals again (this unsets the display extender settings).

Rerolled the patch as it was not applying.

tom_ek’s picture

Status: Needs work » Needs review

Queuing for testing.

Status: Needs review » Needs work

The last submitted patch, 20: metatag-port_views_integration-2563647-20.patch, failed testing.

tom_ek’s picture

Status: Needs work » Needs review
FileSize
40.43 KB

Patch failed to apply. Fixed that.

Status: Needs review » Needs work

The last submitted patch, 23: metatag-port_views_integration-2563647-21.patch, failed testing.

tom_ek’s picture

Status: Needs work » Needs review
FileSize
19.97 KB

Sorry for that "patch storm"... but I have forgotten to remove the debug from the code...
I`ll try to look into why tests fail next.

Status: Needs review » Needs work

The last submitted patch, 25: metatag-port_views_integration-2563647-25.patch, failed testing.

tom_ek’s picture

Ugh. Sorry again. Don`t really understand how that happened. Now the correct file.

tom_ek’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 28: metatag-metatag-port_views_integration-2563647-27.patch, failed testing.

tom_ek’s picture

I have just realized that the thing is throwing warnings like a madman. Did not know that they are hidden by default in D8 :).

The last submitted patch, 31: metatag-port_views_integration-2563647-31.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 31: metatag-port_views_integration-2563647-31.patch, failed testing.

tom_ek’s picture

Adding a fix for the DC problem ( https://www.drupal.org/node/2788449 ) - for those (if any) who need that now :) - just temporarily removing the services. This can be removed once the issue is fixed.

martins.bertins’s picture

In case anyone needs this. Here's a patch from #31 rerolled against 1.0-beta9.

martins.bertins’s picture

Found a bug that was left over from the older patch with which I started the reroll.
This is still for 1.0-beta9.

gaurav.goyal’s picture

Status: Needs work » Needs review

Setting the issue to needs review status.

The last submitted patch, 34: metatag-port_views_integration-2563647-34.patch, failed testing.

caspervoogt’s picture

I tried metatag-port_views_integration-2563647-34.patch but it fails on metatag.services.yml.

gaurav.goyal’s picture

Patch rerolled against dev version of this module.

Status: Needs review » Needs work

The last submitted patch, 40: views_integration-2563647-40.patch, failed testing.

monika.de’s picture

blazey’s picture

Status: Needs work » Needs review
FileSize
382 bytes
55.49 KB

Patch from #42 no longer applies. Attaching rerolled patch with one addition: schema definition for metatag_display_extender.

Status: Needs review » Needs work

The last submitted patch, 43: views_integration-2563647-43.patch, failed testing.

cgmonroe’s picture

Status: Needs work » Needs review
FileSize
56.05 KB

The patch in #44 is failing because it does not include the update to the metatags.services.yml that match the new parameter required in the construct method.

Here's a patch build against dev with that included. Applied and test locally on my site's beta11 install.

Status: Needs review » Needs work

The last submitted patch, 45: views_integration-2563647-44.patch, failed testing.

DamienMcKenna’s picture

Will give this a good look for the 1.0-rc1 release.

kducharm’s picture

Rerolled patch for 1.x-dev 7c1fb3791beac87c9b92b0805c521f285acd9b2b

cgmonroe’s picture

Status: Needs work » Needs review

Changing status to needs review so new patch gets tested.

The last submitted patch, 42: views_integration-2563647-42.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 48: views_integration-2563647-48.patch, failed testing.

danquah’s picture

I had problems applying the patch in #48 (possible due to a newline added before EOF), did a re-roll and removed the newline which at least now applies locally.

Status: Needs review » Needs work

The last submitted patch, 52: views_integration-2563647-52.patch, failed testing.

DamienMcKenna’s picture

Issue tags: +Needs tests
DamienMcKenna’s picture

This also needs some minor polish to match the Drupal coding standards.

paulmckibben’s picture

The patch from #52 no longer applies due to recent changes in the 8.x-1.x-dev branch. I've rerolled that patch so it applies. No other changes.

I agree that it needs polish for coding standards.

Otherwise, I have tried the patch and it works: you can define custom metatags for each view and display. You can translate them using configuration translation. It seems to accomplish my needs quite well.

paulmckibben’s picture

I'm also concerned about the number of TODOs in the patch. Are any of them essential for this functionality? Will they result any any fundamental changes that would cause problems if I were to use this patch on a production site?

paulmckibben’s picture

Status: Needs work » Needs review

Changing to Needs Review to get a fresh run of the tests.

Status: Needs review » Needs work

The last submitted patch, 56: views-integration-2563647-56.patch, failed testing.

danquah’s picture

Attached update to patch #56 adds check to avoid a notice when the view does not have any metatags.
Setting to needs review to see if it fixes any of the errors.

Status: Needs review » Needs work

The last submitted patch, 60: views-integration-2563647-60.patch, failed testing.

Alex Bukach’s picture

Status: Needs work » Needs review
FileSize
56.02 KB

Here's the patch #60 re-rolled against the latest version. I confirm it works for me (tests are still missing).

Status: Needs review » Needs work

The last submitted patch, 62: metatag-views-integration-2563647-62-D8.patch, failed testing.

Alex Bukach’s picture

Oops, sorry, my version wasn't the latest. My bad.

stella’s picture

I haven't had much success with this patch.

I can't get this patch to work, either by applying it (or older versions) against the stable or dev releases. I get the ability to metatags per view display when I enable the module, but upon save, I get returned to the list of Views metatags and my newly saved set is not there. In addition, none of the metatags I did save are rendered on the view page.

stella’s picture

Ok it works if I edit the view and add metatags there. Just doesn't work if i try and add the metatags at admin/config/search/metatag/views, but once I edit the view, I can see a list of views there.

monika.de’s picture

Rerolled patch for 8.x-1.0 version

DamienMcKenna’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 67: 2563647-metatag-views-integration-67.patch, failed testing.