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

Comments

marcoscano created an issue. See original summary.

marcoscano’s picture

I 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?

marcoscano’s picture

Status: Active » Needs review
StatusFileSize
new816 bytes

Since the patch is quite trivial, I went ahead and generated it, but still interested in hearing from the maintainers on the preferred approach.

damienmckenna’s picture

Thank 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.

damienmckenna’s picture

marcoscano’s picture

Status: Needs work » Needs review
StatusFileSize
new996 bytes

👍 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 @deprecated message? I added a few words here as an example.

damienmckenna’s picture

StatusFileSize
new1.76 KB
new1.61 KB

Thanks again.

I updated the change notice, and added some comments to the deprecated function, for anyone who wants to read it.

Status: Needs review » Needs work

The last submitted patch, 7: metatag-n3363366-7.patch, failed testing. View results

damienmckenna’s picture

Status: Needs work » Needs review

That test failure was a random problem that shows up occasionally.

damienmckenna’s picture

Title: metatag_generate_entity_metatags() no longer accepts NULL, causing BC-breaking change » metatag_generate_entity_metatags() no longer accepts NULL, causing BC-breaking change.
Status: Needs review » Fixed

Committed. Thank you for pointing out this accidental BC break.

Status: Fixed » Closed (fixed)

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