Implement hook_dfp_tag_alter()

Reviewing

Hello Dinesh,

#7 Patch applied successfully.

After adding the hook_dfp_tag_alter function the ads size altered

/**
* Implements hook_dfp_tag_alter().
*/
function hook_dfp_tag_alter(&$tag) {
if($tag->id() == 'imu') {
$tag->setSize("300x600,300x250");
}
}

Please check the attachments Before and After how it changed after hook dfp tag alter

Hooks hook_dfp_tag_load_alter & hook_dfp_tag_alter are not defined in dfp.api.php.

If you are adding new hooks, please update

• dfp.api.php

Also, what is the purpose of newly added functions in the patch?

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!

I also can't see a good use case for hook_dfp_tag_load_alter() now using config entities. So if anyone needs it, we should solve that as separate follow-up.

Current MR as a static patch until it is merged.

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

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....

UPDATE: Seems I misunderstood the issue. Adjusting the tests accordingly.

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.

EDIT: The newly added tests succeed.

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.

Static patch attached, until this is merged!

Switching to 3.0.x

Need to retest in v3.

Thanks @vladimiraus - @grevil could you have a final look in the next weeks please?

Seems to work great on v3! I fixed the remaining cspell issues and now the pipeline also succeeds again! 🙂👍

Confirming RTBC! 🚀

@vladimiraus any chance to finally merge this into 3.0.x and tag a new release with this? Would be fabulous 🎉

Latest state of MR39 attached until this is merged. Would be great to have this fixed.

Can once again confirm this still works perfectly in our projects and is definitely needed.

Thank you @vladimiraus!! Great to have the hook back now!!