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.

dkre’s picture

What is required to get this over the line (in approx hours)?

I'm looking into the possibility of sponsorship.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
58.45 KB

Reroll.

DamienMcKenna’s picture

Status: Needs review » Needs work

So at the very least it needs some tests. There are some @todo items listed which could probably be dealt with as follow-on issues. Ultimately, though, I think tests are the main thing left for now.

kylebrowning’s picture

Why are the tests failing?

stijn.blomme’s picture

I tested the patch in #71 and the metatags are added to the views page.

It is possible to add a translation trough the metatag ui.
But the translation is not used. The default language is always rendered.

stijn.blomme’s picture

[double comment]

stijn.blomme’s picture

Small addition to the patch in #71 to allow multilingual views tags.

$language_manager = \Drupal::getContainer()->get('language_manager');
      $langcode = $language_manager->getCurrentLanguage()->getId();
      $tags = $extenders['metatag_display_extender']->getMetatags($langcode);
      // Account for the future lanuage handling.
      $tags = !empty($tags) ? $tags[$langcode] : [];
DamienMcKenna’s picture

Status: Needs work » Needs review

Thanks @stijn.blomme. Lets see what the testbot says.

Status: Needs review » Needs work

The last submitted patch, 76: metatag-viewsintegration-2563647-76.patch, failed testing.

Chris Burge’s picture

It looks like a lot of code disappeared between #71 and #76. Among other things, the Metatag: Views module is missing.

ckaotik’s picture

Status: Needs work » Needs review
FileSize
54.46 KB

Thanks a ton to all of you for getting this so far along!

I've worked on the translatability. Since Views are essentially config entities (with ViewsExecutable black magic), they are easily translated using Core's config_translation once the config schema is configured. I've also retained the separate translation form so users with metatag admin permissions can translate them right there, without needing to dig through 8 levels of details elements :)

I've built the patch against 8.x-1.x-dev (post 8.x-1.0), but am unsure about some of the parts previously added for this issue, e.g. MetatagManager::getDefaultMetatags or MetatagManager::getSpecialMetatags. The same goes for things like config_translation_manage_form_element.html.twig.
Sorry for there being no interdiff, it didn't like working with the different base versions.

Are they still needed? We still have to clean up a bit and add tests, but all things considered I think we are pretty far along the way.

ckaotik’s picture

Updated the previous patch, fixed a whitepage and added tabs for quick navigation between edit/translate.

DamienMcKenna’s picture

WIP on tests for the actual functionality.

DamienMcKenna’s picture

Status: Needs review » Needs work

When I run the tests locally it works up until the point it tries to save the view with two overridden meta tags and then hits this error:

InvalidArgumentException: The configuration property display.page_1.display_options.display_extenders.metatag_display_extender.metatags.robots.index doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 76 of core/lib/Drupal/Core/Config/Schema/ArrayElement.php).
Drupal\Core\Config\StorableConfigBase->castValue('display.page_1.display_options.display_extenders.metatag_display_extender.metatags.robots.index', 0) (Line: 213)
Drupal\Core\Config\StorableConfigBase->castValue('display.page_1.display_options.display_extenders.metatag_display_extender.metatags.robots', Array) (Line: 213)
Drupal\Core\Config\StorableConfigBase->castValue('display.page_1.display_options.display_extenders.metatag_display_extender.metatags', Array) (Line: 213)
Drupal\Core\Config\StorableConfigBase->castValue('display.page_1.display_options.display_extenders.metatag_display_extender', Array) (Line: 213)
Drupal\Core\Config\StorableConfigBase->castValue('display.page_1.display_options.display_extenders', Array) (Line: 213)
Drupal\Core\Config\StorableConfigBase->castValue('display.page_1.display_options', Array) (Line: 213)
Drupal\Core\Config\StorableConfigBase->castValue('display.page_1', Array) (Line: 213)
Drupal\Core\Config\StorableConfigBase->castValue('display', Array) (Line: 212)
Drupal\Core\Config\Config->save() (Line: 280)

DamienMcKenna’s picture

So I rebuilt my local D8.4 install, manually ran through the steps again, and the error is thrown again when the view is saved with modified meta tags.

I also noticed that even opening the Metatag dialog and clicking "Apply", without making any changes, marks it as "overridden", so some work needs to be done here.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
58.91 KB
1.09 KB

This might just work.

DamienMcKenna’s picture

Adding a new trait to ultimately make the tests more reusable.

DamienMcKenna’s picture

This is AWESOME! Thank you all so much for helping to get this functionality working!

Can some others please help test this out with 8.2, 8.3 and 8.4 and let me know if there are any problems. I'd also like to know what scenarios people need so that we can decide on some other tests to write.

As it is I'm going to get this into 8.1, due for release before the end of the month, but would love to hear back on the above.

Thanks again everyone!

ckaotik’s picture

InvalidArgumentException: The configuration property display.page_1.display_options.display_extenders.metatag_display_extender.metatags.robots.index doesn't exist

I think the root cause of this is that robots is marked in metatag's config schema as type: label. However, the data stored in the end is not a string but instead a sequence of booleans (i.e. checkboxes for the individual flags).

I think we should preferrably fix the schema of that tag in config/schema/metatag.metatag_tag.schema.yml from

metatag.metatag_tag.robots:
  type: label
  label: 'Robots'

to

metatag.metatag_tag.robots:
  type: sequence
  label: 'Robots'
  sequence:
    type: boolean
    translatable: true

This should then resolve the error message and we can rely on the metatag mapping key again :)

Edit: I've added the translatable flag so we can still override the robots settings per language (the label type was inherently translatable). However, we might still run into the issue of #2866303: UI missing for translatable data type unless form_element_class is set, but that's on core to fix,

DamienMcKenna’s picture

Good idea, thanks! So, something like this?

Status: Needs review » Needs work

The last submitted patch, 90: metatag-n2563647-90.patch, failed testing.

DamienMcKenna’s picture

@chaotik: Any thoughts on how to fix the errors?

ckaotik’s picture

Hrm, good question. The tests don't seem to work for the new config schema. I'd guess the form submission in MetatagTagsTestBase::testTagsArePresent in line 180 fails because the browser interprets TRUE as a string instead of boolean, maybe change the expected value 1 instead?

  /**
   * Implements {meta_tag_name}_test_value() for 'robots'.
   */
  public function robots_test_value() {
    return TRUE; // Try 1 here instead for form submission.
  }

Try and do this in MetatagAdminTest::testDefaults as well:

    // Test the Robots plugin.
    $this->drupalGet('admin/config/search/metatag/global');
    $this->assertResponse(200);
    $robots_values = ['index', 'follow', 'noydir'];
    $values = [];
    foreach ($robots_values as $value) {
      $values['robots[' . $value . ']'] = TRUE; // Try 1 here instead for form submission.
    }
    $this->drupalPostForm(NULL, $values, 'Save');
DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
60.82 KB
951 bytes

Trying this out.

DamienMcKenna’s picture

Thinking on it some more, I might commit #87 and spin off rearranging the Robots meta tag into a different issue.

Status: Needs review » Needs work

The last submitted patch, 94: metatag-n2563647-94.patch, failed testing.

ckaotik’s picture

Just noticed that I incorrectly documented hook_metatags_alter. Second argument must be array $context, not an entity.

The problem with changing the Robots metatag somewhere else is that we'll trigger a whitepage whenever saving a View. So we'd have to still change the data type in the schema, even if the tests do not pass :/

ckaotik’s picture

Status: Needs work » Needs review
FileSize
8.79 KB
61.23 KB

Soooo, I've been playing around with schema_metatag and finally realized my misconception about the robots metatag. All supported metatags are stored as strings, and the tag manager triggers each tag to convert their values appropriately.
Thus, I've changed the robots type back to string and reworked the display extender to also trigger this handling. Let's see if this works!

  • DamienMcKenna committed de662d5 on 8.x-1.x authored by delta
    Issue #2563647 by delta, tom_ek, DamienMcKenna, monika.de, ckaotik,...
DamienMcKenna’s picture

Status: Needs review » Fixed

Committed! Woohoo! Thanks everyone!

DamienMcKenna’s picture

Lets do all additional changes / requests as separate issues.

kylebrowning’s picture

Thanks for this, works like a charm.

dkre’s picture

So awesome to see this completed!

Thank you to everyone who contributed.

Mytko Enko’s picture

There is still something wrong with the integration in new release of metatag, see my comment on other issue below. Is there a way to fix it?
https://www.drupal.org/node/2883088#comment-12112407

Same happened to me when I did "update" via UI. But when I did it manually - deleted the folder from modules and than extracted new version - after cache rebuild everything is updated and previous settings are working. But another problem occures - when I try to add a new "View meta tag" process crashes after pressing submit button. It doesn't affect on site, but the "view meta tag" is not created and not working.

Reproduce method:

1.
Installing new drupal 8.3.2 site
installing token and metatags - works fine.
Adding new view, attempt to add meta tags to view - process crashes

2.
Manual update of metatag module on development site
enabling views meta tags - everything works as should.
Trying to add view meta tag - process crashes.

The log message in both situations is:
InvalidArgumentException: The configuration property display.page_1.display_options.display_extenders.metatag_display_extender.metatags.robots.index doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 74 of C:\web\xampp\htdocs\mtk-dev\core\lib\Drupal\Core\Config\Schema\ArrayElement.php).

Peacog’s picture

@Mytko Enko I had the same problem as you, and it turned out that an earlier version of the patch had added additional metatags.robots config to the display_extenders.metatag_display_extender.metatags part of the view configuration. To fix it I removed the display_extenders key from the view.yml file, reimported the config and set up the metatags again. It might also work if you reset the view metag to default and then override it again (I didn't try that)

Mytko Enko’s picture

@Peacog Thank you a lot, but I'm a new to drupal and for me your method is a bit hard to achieve. I tested it in clean install of Drupal 8.3.2 and it didn't work, so I am going to create a new issue. Thanks

ckaotik’s picture

I've added a reference to the issue created by @Mytko Enko

Status: Fixed » Closed (fixed)

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