Problem/Motivation

While working on resolving the remaining test failures in #2429617-75: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), I noticed that TrustedHostsTest has a failure. Specifically, in this test:


  protected function setUp() {
    //…
    $this->drupalLogin($admin_user);
  }

  public function testShortcut() {
    $this->container->get('module_installer')->install(['block', 'shortcut']);
    $this->rebuildContainer();

    //…

    $this->drupalPlaceBlock('shortcuts');

    $this->drupalGet('');
    $this->assertLink($shortcut->label());
  }

What happens here, is that we first log in, which causes a SmartCache cache item to be created. But that cache item is never invalidated, even though the Block module will select a different page display variant!

Proposed resolution

Because block module implements a custom page display variant, *and* it does not require any configuration to take over page rendering, it should invalidate the 'rendered' cache tag, to ensure all cached output is invalidated.

Remaining tasks

Do it.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#10 interdiff.txt2.13 KBwim leers
#10 block_installation-2429617-9.patch2.28 KBwim leers
#10 block_installation-2429617-9-test-only.patch1.28 KBwim leers
#1 block_installation-2429617-1.patch949 byteswim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new block_installation-2429617-1.patch949 bytes
fabianx’s picture

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

RTBC - if tests pass

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: block_installation-2429617-1.patch, failed testing.

Status: Needs work » Needs review

Fabianx queued 1: block_installation-2429617-1.patch for re-testing.

fabianx’s picture

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

Test fluke

xjm’s picture

xjm
she/her
Primary language English
commented

Please, when you click retest, document what the test failure was. :)

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks @Fabianx and @Wim Leers. The change makes sense. Yay for fewer cache invalidation bugs! However, I think we should probably add test coverage that the cache is invalidated? I see that #242961: Download system? would add test coverage (per the summary), but best to add a focused test with this fix, I think.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

I honestly don't see the value of adding test coverage for this. It is a single hook implementation, and that implementation does a single function call. Both the hook being invoked and the function call's consequences are thoroughly tested. This would therefore then only be testing that this single function call is being invoked. But we know that it is being invoked.

Here, the cost of writing test coverage outweighs the benefits, IMHO.

Back to RTBC for more committer feedback.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

hook_modules_installed() runs when any module is installed.

This should either be in hook_install() or at least check that block is in the list no?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new block_installation-2429617-9-test-only.patch1.28 KB
new block_installation-2429617-9.patch2.28 KB
new interdiff.txt2.13 KB

Clearly I'm an idiot, because I even managed to get this trivial patch wrong. Which completely undermines my arguments in #8. In other words: I've made a fool of myself :)

Here's a fixed patch, with test coverage.

The last submitted patch, 10: block_installation-2429617-9-test-only.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

That looks better :)

  • catch committed b6fa1ca on 8.0.x 
    Issue #2493091 by Wim Leers: Installing block module should invalidate...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

So the test is fine to make sure the hook_install() works, but it wouldn't test for the issue in #10 where this happens on any module install rather than just block. However testing that the cache isn't invalidated when another module is installed definitely would be too much testing overhead.

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented 
+++ b/core/modules/block/block.module
@@ -306,3 +307,13 @@ function block_configurable_language_delete(ConfigurableLanguageInterface $langu
+function block_install() {

Why is the install hook in .module file and not in .install file?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Created #2535284: Move block_install hook to block.install file.

effulgentsia’s picture

effulgentsia commented
Related issues: +#2783791: Module install doesn't invalidate render cache 
+++ b/core/modules/block/block.module
@@ -306,3 +307,13 @@ function block_configurable_language_delete(ConfigurableLanguageInterface $langu
+  // Because the Block module upon installation unconditionally overrides all
+  // HTML output by selecting a different page display variant, we must
+  // invalidate all cached HTML output.

Block module is not the only module that influences what is rendered. Quickedit, RDF, and many other modules do as well. Therefore: #2783791: Module install doesn't invalidate render cache.