We've noticed incredibly frequent rebuilds of data in cache_discovery for a site I'm working on with a lot of editor traffic. In tracing it back, we realized that \Drupal::entityTypeManager()->clearCachedDefinitions(); was being called on every authenticated request that built a node edit/create form. Digging deeper, we discovered this came from this module, in the xmlsitemap_get_link_info(). This function retrieves information about the various entity types and bundles that could be added to the sitemap. This data is cached, because it's expensive to build. There is also an escape hatch on the function - a $reset parameter that causes the cache to be rebuilt. The problem is twofold:

  • xmlsitemap_add_form_link_options() - the function that adds the XML Sitemap settings to an entity form, calls xmlsitemap_get_link_info() with $reset = TRUE. This causes the info cache to be rebuilt anytime an editor hits the node form.
  • Using the $reset = TRUE parameter actually causes a call to \Drupal::entityTypeManager()->clearCachedDefinitions();, causing a lot of extra data to get cleared out of cache_discovery.

Steps to reproduce

  • Install XML Sitemap and enable it for one bundle of content entities.
  • Note the "created" time for the entity_field_map cache entry in the "cache_discovery" table.
  • Go to a node edit/create form for a node of the bundle you enabled earlier.
  • Observe that the 'created' timestamp for the 'entity_field_map' cache entry has been updated.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rbayliss created an issue. See original summary.

rbayliss’s picture

Minimal patch to fix the behavior.

rbayliss’s picture

Priority: Major » Critical

I'm not 100% sure what edge case the $reset = true and clearing of cached definitions was added for, but I'd argue that those things should be handled elsewhere. In my testing, I wasn't able to replicate a situation where the info wasn't updated following a change to the XML Sitemap settings, or to the bundles themselves.

I'm bumping this to critical, because the scope of the data that gets cleared here is massive - virtually every subsystem in Drupal 8 depends on those entity type definitions being available, so on a site with a lot of editor traffic, this is going to have a massive impact.

rbayliss’s picture

Status: Active » Needs review
FileSize
923 bytes

For those running 8.1-alpha2, here is a patch that can be applied. This version of the patch isn't the one that should be committed though. See comment #2 for that version.

dpi’s picture

Fast forward, work on 79cfd857.

No other changes.

This is a really horrible bug. clearCachedDefinitions shouldnt be abused like this.

+1 on critical state

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, this looks ready to me.

renatog’s picture

Really works good. +1

  • RenatoG committed 4be577a on 8.x-1.x authored by rbayliss
    Issue #2924422 by rbayliss, dpi, jibran, RenatoG:...
renatog’s picture

Status: Reviewed & tested by the community » Fixed

Committed to the dev branch.

Best,

Status: Fixed » Closed (fixed)

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