It's only caching newly found hooks if they have an implementation, leading to those hooks being rechecked again on every request. With a decent number of modules enabled this is 5% of every request being wasted, and it's a one line fix, so marking critical/major.

CommentFileSizeAuthor
module_implements_cache.patch1.27 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Stevel’s picture

Stevel CreditAttribution: Stevel commented
Status: Needs review » Reviewed & tested by the community

Looks good.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Nice catch, catch! ;)

Quick question: Is it possible to write tests for this?

Dries’s picture

Dries CreditAttribution: Dries commented
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: -Performance +Needs tests

Committed to CVS HEAD. Lowering priority and updating tags. Good catch, catch.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Priority: Normal » Critical
Status: Needs work » Reviewed & tested by the community

Bumping back up in priority and reseting status as this doesn't appear to have been committed.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Priority: Critical » Normal
Status: Fixed » Needs work

And setting back to open and normal now we need tests.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: module_implements() caching is broken » (Tests for) module_implements() caching is broken
Category: bug » task

I think it's just about possible to write tests:

* Have two page callbacks.
* One doesn't invoke any hooks.
*One does invoke some hooks that don't have any implementations
* Visit both pages in order, on visiting the second page, do a raw cache_get() of the module_implements() cache and ensure that the hooks with no implementations are in there.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Priority: Normal » Major

Changing to major as per tag.

MustangGB’s picture

MustangGB CreditAttribution: MustangGB commented

Tag update

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Major » Normal

Downgrading, adding tests doesn't meet the criteria for major.

  • Dries committed 0a5d9dd on 8.3.x 
    - Patch #836630 by catch: module_implements() caching is broken.

  • Dries committed 0a5d9dd on 8.3.x 
    - Patch #836630 by catch: module_implements() caching is broken.

  • Dries committed 0a5d9dd on 8.4.x 
    - Patch #836630 by catch: module_implements() caching is broken.

  • Dries committed 0a5d9dd on 8.4.x 
    - Patch #836630 by catch: module_implements() caching is broken.

  • Dries committed 0a5d9dd on 9.1.x 
    - Patch #836630 by catch: module_implements() caching is broken.