Implement hook_dfp_tag_load_alter() and hook_dfp_tag_alter()

I don't think hook_dfp_tag_load_alter(), makes sense in D10.

Done. Please review the provided MR!

Thanks @Grevil! Looks and works great!

Current MR as a static patch until it is merged.

Just found an issue with this implementation: 🐛 Tag altering is not represented in caching (= wrong ads targeting shown) Needs work

While it's super important to have the flexibility this hook provides, it doesn't care for caching.
I'm not even sure if the issue described doesn't also happen in other cases already, but at least for this MR the differences in targeting for the same tag need to be taken into account!

We may decide to merge that other issue in here and fix it here. But first we need to find a correct solution.

Any ideas everyone here?

I added a test trying to replicate the caching issue, but everything seems fine, maybe checking the response isn't the correct approach.

FYI: The ci-pipeline is a complete mess in dev. The pipeline errors are unrelated, the added test succeeds.

Yea, I don't know LGTM 🤔

See https://developers.google.com/publisher-tag/reference#googletag.PubAdsSe....

Nice, now we have a failing test reproducing this issue!

Ok just found this: https://www.drupal.org/docs/drupal-apis/cache-api/cache-max-age#s-what: →

\Drupal\Core\Cache\Cache::PERMANENT (value -1) means cacheable forever, i.e. this will only ever be invalidated due to cache tags. (In other words: ∞, or infinite seconds.)

@Anybody and I took a deeper look into this whole endeavour. The problem to begin with was, that "TagViewBuilder" did not add cache keys based on the dfp tag targeting (as this wasn't necessary before). Even after adding them, the caching wasn't fixed.

Then we found out, that the TagBlock "build()" wasn't executed again after reloading the page.

Turns out, the caching logic on the block hindered us to even reach the TagViewBuilder "build()" method, as the caching keys set in the TagViewBuilder do not bubble up to the block (caching keys do not bubble up in general, where as cache context does). We tried to simply add caching keys to "TagBlock" build(), but this only added cache keys to the block content, not to the block itself. We then tried to implement "hook_block_build_BASE_BLOCK_ID_alter" to add the proper caching keys to the block (see here).
This worked perfectly, but unfortunately the hook fires before the content gets passed to the block, meaning we do not have any information about the dfp tag in the alter hook.

Now in the final approach, we simply set the block cache max age to "0", IF the alter hook is implemented. Otherwise, the original max age is used. This way (if the alter hook is implemented) we delegate the block level caching (TagBlock) to the content level caching (TagViewBuilder).
Both @Anybody and I feel like this is the best approach, (since the approach through "hook_block_build_BASE_BLOCK_ID_alter" isn't working).

Another approach would be, to simply create a new "dfp_tag_targeting" cache CONTEXT, as cache contexts simply bubble up compared to cache KEYS. But a cache context for this specific scenario seems like the incorrect approach:

Cache contexts = (request) context dependencies

( https://www.drupal.org/docs/drupal-apis/cache-api/cache-contexts → )

Dynamic tag targeting through the hook implementation doesn't have much to do with "REQUEST context dependencies". As it has nothing to do with the request itself (unlike other core contexts like "theme","cookies","language", etc.) and there can be multiple seemingly identical blocks with different tag targeting in one request. Maybe this is an edge case, where it is fine to create a dedicated cache context, even if it isn't linked to a "Vary header" (request cache context), but this isn't documented anywhere, whether there are edge cases like this.

Please review.

Whao that was pain... but with happy end! :)

Thank you for the great investigations we had together @grevil! So I can happily confirm this is RTBC!
This is tested, tests fail without the cache fixes and so we can be sure it works as expected.

If we'll ever come to a better solution, which I currently can't see by hard, we can do that.

Other failing tests / broken pipeline is unrelated, as written above.

anybody → changed the visibility of the branch 3220030-approach-bubble-up-keys-to-block to hidden.

Static patch attached, until this is merged!