Title: Tag cache metadata is discarded, so tags are cached
without their own cache tags
TagViewBuilder::viewMultiple() throws away the result of
CacheableMetadata::merge():
// src/View/TagViewBuilder.php, line 132 in 3.0.0 $cacheable_metadata = CacheableMetadata::createFromObject($global_settings); $cacheable_metadata->merge(CacheableMetadata::createFromObject($tag)); $cacheable_metadata->addCacheTags($this->getCacheTags()); $cacheable_metadata->applyTo($build[$tag_id]);
merge() returns a new
CacheableMetadata rather than modifying the object in place.
Discarding the return value means the second line does nothing, and only the
global settings' cache metadata reaches applyTo().
The practical effect is that a rendered tag does not carry the cache tags of
the dfp_tag config entity it was built from. Editing a tag will
not invalidate a render cache entry that was built from it, so a stale ad tag
can be served until something else clears the cache.
Present in 3.0.0, and on 3.0.x, 2.0.x and
8.x-1.x. It is not new and not specific to any Drupal version —
Drupal 12 simply started saying so, because
CacheableMetadata::merge() is now marked as a method whose return
value must not be discarded:
User warning: The return value of method Drupal\Core\Cache\CacheableMetadata::merge() should either be used or intentionally ignored by casting it as (void) Drupal\dfp\View\TagViewBuilder->viewMultiple() (Line: 133)
That is how it was found: the next major test job on
#3619663
turned it from a silent no-op into eight failing functional tests.
$cacheable_metadata = CacheableMetadata::createFromObject($global_settings) ->merge(CacheableMetadata::createFromObject($tag));
The fix is already in
>MR !46
>
on #3619663, because that branch cannot go green without it. This issue exists
so the bug is recorded on its own terms rather than buried in a compatibility
ticket, and so it can be considered for backport to
2.0.x.
2.0.x.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare
Comments
Comment #3
marcelovani>MR !47 >
is ready. Could another maintainer review? I would rather not merge my own
work.
The change
One line in
TagViewBuilder::viewMultiple().CacheableMetadata::merge()returns a new object rather thanmodifying the one it is called on, and the return value was being dropped, so
the merge did nothing. Measured on a built tag, before and after:
The tag's own config cache tag is the one that goes missing, so editing a tag
did not invalidate a render cache entry built from it.
The test
tests/src/Kernel/TagViewBuilderCacheTestis new, and is the firstkernel test in the module. Nothing asserted that a tag's cache tags reach the
build, which is why this went unnoticed. I checked it is a real regression
test rather than a formality: it fails with the one line reverted and passes
with it in place.
Please merge #3619663 first
!47 shows a red pipeline, and it is not this change.
3.0.xhas been failing CI since January — the last threepipelines on the branch are all red — and !47 inherits that. Four blocking
jobs fail on it: cspell, phpcs, phpstan and phpunit.
All four are fixed on
#3619663,
where the pipeline is green. Worth knowing that the phpunit one is
ContainerNotInitializedException, fromHtmlResponseAttachmentsProcessorgaining a constructor argument —which means the unit tests already fail on Drupal 11.4 and up, not only on 12.
So: merge
!46
first, then rebase this one and it goes green. The same one line fix is in
!46, because that branch cannot pass without it — newer core marks
merge()as a method whose return value must not be discarded, andthat turned this silent no-op into eight failing functional tests. After the
rebase !47 is effectively just the regression test.
If you would rather have it the other way round, the fix and the test could
move here and come out of !46 instead. Either order works, they just cannot
both be reviewed in isolation.
Tested
Passes on 10.6.15 and 12.0-dev.
Comment #4
marcelovani