Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gam): better handling of broken ad unit #934

Merged
merged 3 commits into from
Jan 17, 2025

Conversation

miguelpeixe
Copy link
Member

GAM may fail to create the default ad units. Our implementation currently doesn't handle that and is storing the default ad units as null and breaking our GAM ad renderer.

Testing

  1. Make sure you have GAM setup with placements using default ad units
  2. Force the default ad unit creation to fail by adding the following to line 300 of includes/providers/gam/api/class-ad-units.php:
return new \WP_Error( 'test_error, 'Test Error' );
  1. Also ensure an attempt to create will be made by preventing it from finding an already created ad unit. Replace line 353 of includes/providers/gam/class-gam-model.php with:
$ad_unit_idx = false;
  1. Then, delete the currently stored data via CLI: wp option delete _newspack_ads_gam_default_units
  2. Visit Newspack -> Advertising to trigger the creation
  3. After the page loads, confirm the options are stored like this:
$ wp option get _newspack_ads_gam_default_units
array (
  'newspack_below_header' => NULL,
  'newspack_sticky_footer' => NULL,
  'newspack_sidebar_1' => NULL,
  'newspack_sidebar_2' => NULL,
  'newspack_in_article_1' => NULL,
  'newspack_in_article_2' => NULL,
  'newspack_in_article_3' => NULL,
)
  1. Visit a page that renders a default ad unit and confirm you get a fatal error
  2. Checkout this branch, repeat steps 2-5 but the stored ad units are stored with the correct local values
  3. Visit a page that renders a default ad unit and confirm the page renders without issues

@miguelpeixe miguelpeixe requested a review from a team as a code owner January 15, 2025 21:21
@miguelpeixe miguelpeixe self-assigned this Jan 17, 2025
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works!

@miguelpeixe miguelpeixe merged commit 0fdcc4f into release Jan 17, 2025
9 checks passed
@miguelpeixe miguelpeixe deleted the hotfix/gam-handle-broken-ad-unit branch January 17, 2025 18:12
matticbot pushed a commit that referenced this pull request Jan 17, 2025
## [3.3.1](v3.3.0...v3.3.1) (2025-01-17)

### Bug Fixes

* **gam:** better handling of broken ad unit ([#934](#934)) ([0fdcc4f](0fdcc4f))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.3.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants