metatag_entity_load() uses metatag_entity_supports_metatags() for querying entities metatags instances, but it could reduce the $revision_ids by first checking if the bundles are enabled, thus potentially reducing the number of SQL queries.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pounard’s picture

Status: Active » Needs review
Issue tags: +Performance
FileSize
2.77 KB

For example, the given patch applied on a module where lives something like 50 node types but only 4 of them are enabled to use metatags (using hook_entity_info_alter()) saves us an average of 30 SQL queries per page, which is significant.

pounard’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.53 KB

And here is a patch for the current version.
Patch has been done with a manual diff, so you need to:

patch -p0 < PATCHFILE

manually to apply. Drush make will apply cleanly.

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Needs review
DamienMcKenna’s picture

FileSize
3.07 KB

@pounard: Thanks for the patch. I've made a few minor tweaks.

pounard’s picture

Looks good to me, glad I could help.

DamienMcKenna’s picture

DamienMcKenna’s picture

FileSize
3.62 KB

Rerolled.

DamienMcKenna’s picture

Status: Needs review » Needs work

This needs work.

DamienMcKenna’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: metatag-n2070821-7.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review
Issue tags: -Performance
FileSize
23.04 KB

While I was rerolling this patch I realized something: A lot of work was being done to deal with the fact that you have to change the API of an entity to indicate it's supported by Metatag, and then there's a settings page which lets you control which one of those entity types is actually enabled. And then there was a whole extra function / internal API for deciding whether entity types should be disabled anyway. And if there wasn't an enabled global configuration for a specific entity there was no way of supporting that entity. It was all a little confusing.

So I've simplified this whole system:

  • The API to control whether entities are supported has been removed.
  • From now on, any entity that has view modes and is not a configuration entity may be enabled or disabled through the Advanced Settings page, with a default of "enabled" (files, comments and fieldable_panels_panes are disabled by default).
  • An update script is provided to move existing sites to this configuration.
  • An install hook is added to define some initial values.
  • The uninstall hook was updated accordingly.
  • There's now a function for enabling support for a specific entity type or bundle: metatag_entity_type_enable()
  • There's now a corresponding function for disabling support for a specific entity type or bundle: metatag_entity_type_disable()
  • All existing tests pass correctly! Woo!!
  • In my own personal testing I was able create meta tags for a node, a term and a user, so the basic functionality appears to still work correctly.
  • This will require a change notice.
  • Life will be simpler going forwards!
  • I don't think if this is really a performance improvement anymore ;-)

Please let me know what you think!

DamienMcKenna’s picture

FileSize
23.04 KB

Some minor adjustments - I accidentally renamed the "Advanced settings" fieldset on the (newly renamed) settings page, and improved the entity type settings' description a little on that page.

The last submitted patch, 12: metatag-n2070821-12.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: metatag-n2070821-12.patch, failed testing.

DamienMcKenna’s picture

The tests pass locally but fail on testbot :(

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
39.71 KB

Well, if I upload the same patch twice then no wonder it fails ;-) Also, I'd accidentally created the patches in the wrong location, so the patch in #12 wasn't the correct one to start with.

*This* is the correct patch file.

DamienMcKenna’s picture

FileSize
40.97 KB

This removes hook_entity_info_alter() entirely, clears the caches at the end of the update script and adds another update script to clear the menu cache because the 'advanced settings' page was renamed to just 'settings'.

  • DamienMcKenna committed b11b55d on 7.x-1.x
    Issue #2070821 by DamienMcKenna, pounard: Major re-architecture to how...
DamienMcKenna’s picture

Status: Needs review » Fixed

Committed. I'll need to do a change notice for this.

DamienMcKenna’s picture

I've written up a change notice: https://www.drupal.org/node/2495819

pounard’s picture

It took quite some time, but thank you very much for commiting this.

Status: Fixed » Closed (fixed)

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