Contrib.social

Tag altering is not represented in caching (= wrong ads targeting shown)

Open on Drupal.org →
Created on 13 March 2024, 9 months ago
Updated 29 August 2024, 4 months ago

Problem/Motivation

Altering dfp tags like in the dfp.api.php:

/**
 * Alter the tag object just after it is loaded for rendering in a block.
 *
 * @param object $tag
 *   The tag object.
 */
function hook_dfp_tag_alter(&$tag) {
  // Adjust a tags add targeting:
  $customTargetDefinition = [
    'target' => 'my_target',
    'value' => 'my_value',
  ];
  $tagAddTargetting = $tag->targeting();
  $tag->set('targeting', array_merge($tagAddTargetting, [$customTargetDefinition]));
}

doesn't affect the cache, so even with different targeting Drupal thinks it's the same dfp tag and returns it from cache?

Steps to reproduce

See above.

Alter a dfp tag to add custom targeting. After cache rebuild only the first returned ad is being rendered with its targeting. All following ads from this tag are returned with that targeting instead of the right one.

This can for example be tested using a block.

Proposed resolution

Cache implementation in TagViewBuilder.php needs to take the different targeting into account for the hook to work as expected.
Current code doesn't seem to do that?

  /**
   * {@inheritdoc}
   */
  public function viewMultiple(array $entities = [], $view_mode = 'full', $langcode = NULL) {
    /** @var \Drupal\dfp\Entity\TagInterface[] $entities */
    $build = [];
    foreach ($entities as $tag) {
      // @todo Ensure a tag is only once on the page.
      // @todo Get cache-ability based on tokens used in TagView...
      $global_settings = $this->configFactory->get('dfp.settings');
      $tag_view = new TagView($tag, $global_settings, $this->token, $this->moduleHandler());

      $tag_id = $tag->id();
      $build[$tag_id] = [
        '#cache' => [
          'keys' => ['entity_view', 'dfp_tag', $tag_id],
        ],
      ];

      // Sort out the cache tags and contexts.
      $cacheable_metadata = CacheableMetadata::createFromObject($global_settings);
      $cacheable_metadata->merge(CacheableMetadata::createFromObject($tag));
      $cacheable_metadata->addCacheTags($this->getCacheTags());
      $cacheable_metadata->applyTo($build[$tag_id]);

      $build[$tag_id] += static::buildPreTag($tag_view);

    }

    return $build;
  }

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Needs work

Version

2.0

Component

Code

Created by

🇩🇪Germany Anybody Porta Westfalica

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Anybody
  • Merge request !17DIRTY QUICKFIX: Implemented quickfix to disable caching on dfp_tags (#3427684) → (Open) created by Anybody
  • Status changed to Needs work 9 months ago
  • Comment 9 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    Just verified this. With the quickfix the ads targeting is now correct.

    This needs to be solved cleanly.

    This is an issue for anyone using the hook as described in the .api.php file.

  • Comment 9 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    Looking into this, I think the right way might be to implement / override

    Tag::getCacheTags()

    as this is being called in TagViewBuilder:

    CacheableMetadata::createFromObject()

    That method should then include cache tags for all relevant (varying) attributes like $targeting, I think?

    What do the maintainers say? Am I right or any better way?

  • Merge request !18Tried to implement a fix, but doesn't seem to work yet. → (Open) created by Anybody
  • Comment 9 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    In MR!18 I tried implementing a fix, but it doesn't seem to work. Happy to get some feedback and ideas how to solve this best.

  • Comment 9 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    Quickfix as static patch attached!

  • Comment 4 months ago →
    🇩🇪Germany Grevil

    Had a quick chat with @alexpott on slack, he suggests creating a test, which reproduces the issue so it is easier to get into the in's and out's.

  • Comment 3 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    I modified the MR!17 quick fix a bit, as we found out that the $cacheable_metadata part was overriding our max-age value. If we set it afterwards, it also works and is much simpler.

    So it seems the max-age = -1 set by the $cacheable_metadata is causing the array to be never-reevaluated even with different keys...

  • Comment 3 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    @Grevil: Looking at several core examples, I come to the conclusion, that our cache keys are the correct approach, but this line may cause the trouble:

    $cacheable_metadata = CacheableMetadata::createFromObject($global_settings);

    as I think that will add the unwanted max-age=-1?
    For me it makes sense, that something static like the settings may be cached forever... should we really use this as basis?

    So for the next step I'd says, first check where the max-age=-1 comes from and let's eliminate that as possible root cause. I think the implementation wasn't fully correct.

    Maybe then our keys will already work...

  • Comment 3 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    Okay they won't ... still I think that point is close to the reason...

    So perhaps let's look into core for similar cases and other implementations of CacheableMetadata::createFromObject etc.

  • Comment 3 months ago →
    🇩🇪Germany Grevil

    This is now fixed in #3220030: Implement hook_dfp_tag_alter() → . Everything is documented there.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024