Problem/Motivation

When rendering ads with the ad_content module, a PHP deprecation notice is triggered on PHP 8.1+:

Deprecated function: str_replace(): Passing null to parameter #2 ($replace) of type array|string is deprecated
in Drupal\ad_content\Plugin\Ad\Bucket\AdContentBucket::postAdRender()
(line 252 of /modules/contrib/ad/modules/ad_content/src/Plugin/Ad/Bucket/AdContentBucket.php)

This happens when $build['#ad_impression_id'] is missing or null, which results in str_replace() receiving an invalid argument.

Steps to reproduce

  1. Install and enable the ad and ad_content modules.
  2. Create an ad bucket and render ads (for example via Mercury Editor or an ad placement).
  3. On PHP 8.1+ observe the deprecation warning in logs:
    str_replace(): Passing null to parameter #2 ($replace)...

Proposed resolution

Update AdContentBucket::postAdRender() to safely handle nulls by casting to string and defaulting to an empty string when the #ad_impression_id is not set. Example fix:

$search  = (string) TrackerInterface::PLACEHOLDER_IMPRESSION;
$replace = isset($build['#ad_impression_id']) ? (string) $build['#ad_impression_id'] : '';
$subject = (string) $markup;

$output = str_replace($search, $replace, $subject);

return $markup instanceof \Drupal\Component\Render\MarkupInterface
  ? \Drupal\Component\Render\Markup::create($output)
  : $output;

Remaining tasks

  • [ ] Attach patch with fix.
  • [ ] Review by maintainers.
  • [ ] Add test coverage to ensure postAdRender() works without #ad_impression_id.

Note: I will attach a patch to this issue, as I cannot currently provide a Merge Request on GitLab for this project.

User interface changes

None.

API changes

None (only internal implementation hardened).

Data model changes

None.

CommentFileSizeAuthor
1.diff1.23 KBdobe

Issue fork ad-3544099

Command icon Show commands

Start 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:

Comments

dobe created an issue. See original summary.

anybody’s picture

Assigned: Unassigned » lrwebks

@dobe please use MRs not patches.

@LR: Could you please have a look?

lrwebks made their first commit to this issue’s fork.

lrwebks’s picture

Status: Active » Needs review
lrwebks’s picture

In general, we should perhaps find the cause of why the impression ID was null in the first place. @dobe right now, our solutions are equally correct, but let me take a look at what is actually happening under the hood there.

anybody’s picture

Status: Needs review » Needs work

MR is okay but please first try to find out what causes this, would be better to fix the root cause!

lrwebks’s picture

Assigned: lrwebks » Unassigned
Status: Needs work » Needs review

Found the cause to actually be intended behavior! The impression ID used in the discussed function is returned by the active tracker once it has registered said impression. Though if there is no ad tracker, or the user has the permission “bypass ad tracking” then the impression will of course not be tracked and the function returns NULL.

This MR is still needed for proper handling of that value and avoiding the deprecated functionality, so we should merge this!

Sidenote: This bug very obviously happens once the module is used for the first time, because the default “NULL” tracker is still selected in settings, which returns NULL (at least now, thanks to me fixing that too), because it simply isn't allowed to track anything. And for an admin, the impression ID is always NULL, because they have the “bypass ad tracking” permission enabled by default.

anybody’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!!

anybody’s picture

Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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