Problem/Motivation
It seems like #3332348: Add function for getting list of overridden meta tags for a given entity (released in 8.x-1.23) has introduced a BC-breaking change respect to 8.x-1.22:
In 1.22, this code worked and returned the tags for the current route's entity:
$tags = metatag_generate_entity_metatags(NULL);
In 1.23, the same code produces an error since metatag_generate_entity_all_tags() is typehinted to a ContentEntityInterface parameter.
TypeError: metatag_generate_entity_all_tags(): Argument #1 ($entity) must be of type Drupal\Core\Entity\ContentEntityInterface, null given, called in /var/www/html/web/modules/contrib/metatag/metatag.module on line 634 in metatag_generate_entity_all_tags() (line 646 of modules/contrib/metatag/metatag.module).
metatag_generate_entity_all_tags(NULL) (Line: 634)
metatag_generate_entity_metatags(NULL) (Line: 21)
In practice, this prevents projects where custom code use metatag_generate_entity_metatags(NULL) to upgrade to a D10-compatible version of the module, which is unfortunate.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | metatag-n3363366-7.patch | 1.61 KB | damienmckenna |
| #7 | metatag-n3363366-7.interdiff.txt | 1.76 KB | damienmckenna |
Comments
Comment #2
marcoscanoI can work on a patch, but I'm not sure what the maintainers would prefer... should we remove the type hint and allow
metatag_generate_entity_all_tags()to grab de entity from the route, or should we think of something else?Comment #3
marcoscanoSince the patch is quite trivial, I went ahead and generated it, but still interested in hearing from the maintainers on the preferred approach.
Comment #4
damienmckennaThank you for pointing out that this was an inadvertent API change.
This function was intended to be used to obtain meta tags for a given entity, it was never intended to be used with a NULL argument.
I think the correct approach here would be to expand the change notice to make this more clear, and update metatag_generate_entity_metatags() so that it handles the scenario of $entity being NULL.
Comment #5
damienmckennaComment #6
marcoscano👍 that makes sense, thanks for the feedback. As for clarifying the change in the notice, you mean just changing the text of https://www.drupal.org/node/3332350 , or also including a note in the
@deprecatedmessage? I added a few words here as an example.Comment #7
damienmckennaThanks again.
I updated the change notice, and added some comments to the deprecated function, for anyone who wants to read it.
Comment #9
damienmckennaThat test failure was a random problem that shows up occasionally.
Comment #11
damienmckennaCommitted. Thank you for pointing out this accidental BC break.