Add a TypedData plugin to support Search API

This patch adds a new module to support the search_api. It includes a processor plugin which stores the basic metatags saved to an entity. The same principle could be applied to store other metatags if needed, but thought the basics would be a good start.

I don't see any difference in search results after enabling the module. I suppose the processor should not be visible in the processors list because it's marked as hidden.
It would be nice to be able to boost the different properties because they are probably quite relevant for search as they are used for SEO too.

@mErilainen -- I was able to add meta keywords and descriptions as new index fields after enabling the module, then set the type to fulltext and boosted as needed. Thanks for the patch @tsplash!

Yes, I realized it too that they have to be added as fields! This is perfect, everything can be boosted separately, just what I was looking for.

But the patch has some coding style issues which should be fixed. I was also getting a lot of notices for nodes which don't have any values saved for the metatag field (metatag has been enabled after content was created).

This can be easily fixed by checking if there is something to unsearialize:

<?php
if (isset($metatags[0]['value'])) {
  $metatags = (unserialize($metatags[0]['value']));
}
?>

Here is an updated patch with coding style fixes and the check for values array to avoid notices while indexing.

Not sure what is the best way to implement this. I managed to get all the tags using the metatag.manager service:

$sorted_tags = \Drupal::service('metatag.manager')->sortedGroupsWithTags();

But there is no information about which fields are probably comma separated like the keywords field is for example.

Also I'm not sure if there are just too many tags which will make the Search API UI bloated. In my current setup I have mobile, open_graph and verification sub-modules enabled in addition to metatag core tags and I get 141 tags. I think most of them are improving machine readability of the content and therefore useless for search index, such as image urls, dimensions etc. Most of the tags which actually have human readable content are usually using field tokens which means they can be indexed to Search API using field properties. So some higher level decision should be done how this could be handled, I guess.

Thank you both for the feedback.

Here is an updated patch so the processor plugin is included in the base metatag module (good point there @DamienMcKenna). It also includes mErilainen's improvements (thank you for those).

@DamienMcKenna regarding you second request around dynamically identifying which tags are available, I agree with mErilainen's points. Unless the metatag api provided information on how the values are entered/stored, we will need to include some hard coded logic for many tags. Additionally as we are including this plugin in the base module, this logic could end up referring to tags which exist in the sub-modules. Also agree on the point around sheer number of available tags.

Should each submodule contain a plugin for the metatags it provides, or is this perhaps a bit overkill? For example from the tags provided by the base module, I could possibly see a need for the 'Geographical place name', 'News Keywords' and perhaps the 'Standout' metatags (which we could add) but not much else.

Attached patch is an entire rewrite, which hopefully resolves the issues at hand.

Initially I had used a processor plugin to create the search_api fields dynamically as the metatag FieldType doesn't have the tags mapped as propertyDefinitions. The search_api looks at the propertyDefinitions on a field and makes them available through the UI in a nice collapsible menu. This would be perfect for the amount of tags available and resolves the UI bloating mErilainen was highlighting with the original solution.

From a bit of research other modules have solved this by defining computed properties. This patch does this is a dynamic manner, making all tags available as computed properties.

This does leave open the point around comma delimited tags, as each tag is indexed as a single value. However these could then be split with an additional processor or preprocess function. My gut feeling is that this wouldn't be the responsibility of our integration.

What do you guys think?

Couple mistakes in the previous patch's comments, apologies.

As for use cases, I was about to add a hidden field for "Search result description" to get some text into the search index which would not be visible in the content entity at all. Then I remembered that several pages already had a meta title + description which Google for example displays as the summary when you see the page in the results. And then I found this issue which would solve my problem and didn't need to add another field for metadata.

Also as a special case, we have some pages which have React components which render content from external API or elasticsearch, so they don't actually have much Drupal content other than a title, so this solves also that case to get something into the search index without adding hidden fields.

I think the new approach looks very nice. Here is a patch with some code style fixes.

Fixed patch format.

Thanks for exposing the fields to Search API.

What about providing deeper integration with Search API, like excluding content from the index when the metatag noindex is checked?

There are other ways to exclude items and I would not want to assume that noindex should also exclude content automatically from Search API without control which might be used for internal searches, for authenticated users etc.

In that case I'd say this is RTBC.

I do have these small remarks:

/**
+ * Cached processed value.
+ *
+ * @var string|null
+ */
+ protected $value = NULL;

=> no need to indicate null or set a variable to NULL.

+ if ($definition->getSetting('tag name') === NULL) {
=> why not use is_null()?

This patch seems to have broken on the latest dev release. It worked for our needs and I look forward to it being merged so I can update metatag.

Tried to use patch 2901039-add-support-for-search-api--16.patch. Can now see the meta tag field available in the search api fields listing. Rebuilt the index and re-indexed. But the the search results are not showing anything when searching for that keyword.

Hi @jenniferaube, welcome to drupal.org. I have confirmed your account so you no longer need to enter capcha's when posting comments.

Did you add the field as a full text field?
Have a look at the search view and make sure you checked the field in list of fields to use for the fulltext filter.

I applied the feedback from @mpp in comment #21. But I will leave the === NULL since this seems the preferred way in Core. As long as the check is strict it is good.

I also changed the setting tag name to tag_name (without the space). We don't like seeing spaces in setting keys.

Up next we will add a kernel test for the typedData.

Missed one of the tag name setting keys.  🙈

Looks like part of the patch is missing :-)

I uploaded the wrong file.
(Guess I need coffee...)

I spent the last 4 hours working on this with @wannesderoy and we can't figure out how to write a kernel test for this, especially not because most of things we would be testing would be things that test the typed data layer. We could add a test to check that if we pass in a value to setValue that it gets returned from getValue.

That is again a way to test the typed data api's, and I don't think it makes sense to do that.
We manually tested that this at least exposes typed data and that seemed to be working as expected.

Based on that I'm setting this to RTBC - at least to surface the problem to @DamienMcKenna, but it might be that this is already good enough without testcoverage.

Retitling as well to be more explicit.

#34: @wannesderoy, @borisson_, great work!

#32: @wannesderoy, Happens to the best ;-)

In fact if I had been paying some more attention in #21, I would have noticed this earlier:
+ ->setLabel(t('@label', ['@label' => $tag['label']]))

This feels wrong. I don't have an IDE at hand so I can't immediately see what class the $tag is but we should probably get the label somewhere up in the stack from calling sortedGroupsWithTags (I assume it's the plugin annotation's label?).

Thank you for this patch! However I encountered a couple of issues with it that I wasn't sure how to solve.
1. If a meta tag on a node is using the default value configured at /admin/config/search/metatag, no value is indexed for that field.
2. If a meta tag contains tokens, only the token name is indexed, rather than the content it refers to.

I upload a new version of patch on #32. Changes:

1. I try to solve problems described on #38 driven by #39
2. I fix the typo described on #37. But $this->t() cannot be done because that method is static.

I upload a new patch to remove a debugging line I missed to remove.

@DamienMcKenna Is there a way to render metatags in a more performant way? After my patch, I think the indexing performs much worse.

@DamienMcKenna I tried a cache system approach given I need this feature ASAP and I think the approach you suggested would take me much more time.

@DamienMcKenna With patch on #45, the performance is back to normal. With my test data, with patch on #41 it indexed on 6 minutes and with patch on #45 it indexed on 4 minutes (which is normal).

@DamienMcKenna You are right, there are still performance issues. I just did a performance test and these are the results:

1 minute 12 seconds (patch #32)
3 minutes 50 seconds (patch #45)

@DamienMcKenna Please, could you do your own performance tests? Sometimes, patch on #45 performs much faster. I don't know if there is a special method to measure performance on Search API indexing (maybe I'm doing something wrong).

On the other hand, any clue about how to modify the patch to process all of the tags in one go?

@DamienMcKenna I've repeated performance tests and it seems that performance is better than I thought:

My measurements on #49 were wrong because I did them on a local vagrant multisite environment and I measured times via UI indexing. Now I modified the same multisite environment to run via DDEV (Docker) and I measured times via terminal. Also, In previous analysis I didn't cleared the cache and the index before every measurement (now I do).

Before each run I do:

drush cr
drush search-api-clear

Time measuring command:
drush search-api-index multisite_search_index

Results:

1. 2660 elements to index (same site as #49):

patch #32, 59.28 seconds
patch #45, 1min and 1.15 seconds

2. 10266 elements to index (another site)

patch #32, 1min and 20.58 seconds
patch #45, 1min and 25.79 seconds

Changes from patch on #45:
- Removed cache code on metatag_generate_entity_metatags method. This code was introduced in #45 and produces a language bug from which metatags can be rendered in another language!
- Removed usage of metatag_generate_entity_metatags method to render metatags, custom code is used instead (only renders the desired metatag)

Please, review this new version!

• Added conditions to allow the "href" attribute to be used as the value for the metatag when "content" doesn't exist.
• Added some logging and conditions to catch undefined index notices and improve the troubleshooting experience.

Oops! Tested this patch before upload.

@DamienMcKenna @weekbeforenext Patch #55 works fine!

I'm getting this deprecation warning when I drush cr:
Deprecated: Drupal\metatag\TypedData\Metatags::__construct(): Implicitly marking parameter $parent as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/docroot/modules/contrib/metatag/src/TypedData/Metatags.php on line 31

OK. I made it here. It's my first time making a bug report here, so apologies if its formatted incorrectly.