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 fatal error when fetching marketing campaigns for Marketing Channel #2423

Merged

Conversation

jorgemd24
Copy link
Contributor

@jorgemd24 jorgemd24 commented Jun 6, 2024

Changes proposed in this Pull Request:

When loading the Marketing -> Overview section, there is a request to fetch the campaigns, but it currently fails because the campaign types are not loaded. The campaign types are only loaded if the filter woocommerce_gla_enable_mcm returns true.

The fatal error is the below:

undefined array key "google-ads" in /opt/homebrew/var/www/wp-content/plugins/google-listings-and-ads/src/MultichannelMarketing/GLAChannel.php on line 195, referer: https://civet-pleasing-internally.ngrok-free.app/wp-admin/admin.php?page=wc-admin&path=%2Fmarketing
[Thu Jun 06 16:19:22.787147 2024] [php:error] [pid 2819] [client ::1:56339] PHP Fatal error:  Uncaught TypeError: Automattic\\WooCommerce\\Admin\\Marketing\\MarketingCampaign::__construct(): Argument #2 ($type) must be of type Automattic\\WooCommerce\\Admin\\Marketing\\MarketingCampaignType, null given, called in /opt/homebrew/var/www/wp-content/plugins/google-listings-and-ads/src/MultichannelMarketing/GLAChannel.php on line 194 and defined in /opt/homebrew/var/www/wp-content/plugins/woocommerce-dev/plugins/woocommerce/src/Admin/Marketing/MarketingCampaign.php:68\nStack trace:\n#0 /opt/homebrew/var/www/wp-content/plugins/google-listings-and-ads/src/MultichannelMarketing/GLAChannel.php(194): Automattic\\WooCommerce\\Admin\\Marketing\\MarketingCampaign->__construct('21359526875', NULL, 'Campaign 2024-0...', 'https://civet-p...', Object(Automattic\\WooCommerce\\Admin\\Marketing\\Price))\

Screenshots:

image

This PR prevents the error by checking if the campaign types have been enabled before fetching the campaigns.

Detailed test instructions:

  1. Checkout develop.
  2. Go to Marketing -> Overview.
  3. Check your PHP errors or open the console and see how the campaign request fails.
  4. Checkout this branch.
  5. Refresh the Marketing -> Overview page.
  6. See that we prevent the fatal error.
  7. Add the following filter: add_filter( 'woocommerce_gla_enable_mcm', '__return_true' );
    8 Refresh and see that now the campaigns are loaded.

Additional details:

Tests are failing because of this issue: #2415 (comment) however you can run the tests locally and see if everything is OK

Changelog entry

Fix - Fatal error when loading campaign in the marketing overview section.

@github-actions github-actions bot added type: bug The issue is a confirmed bug. changelog: fix Took care of something that wasn't working. labels Jun 6, 2024
@jorgemd24 jorgemd24 self-assigned this Jun 6, 2024
@jorgemd24 jorgemd24 requested a review from a team June 6, 2024 19:34
@jorgemd24 jorgemd24 marked this pull request as ready for review June 6, 2024 19:39
@jorgemd24 jorgemd24 changed the title Fix fatal error when fetching marketing campaings for Marketing Channel Fix fatal error when fetching marketing campaigns for Marketing Channel Jun 6, 2024
Copy link
Member

@ianlin ianlin left a comment

Choose a reason for hiding this comment

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

Thanks for catching this issue and fixing it. Tested locally, I could no longer see the fatal error and the campaigns were displayed correctly. I'm marking it approved and just left a non-blocker naming suggestion.

Comment on lines 85 to 86
protected function is_enable_campaign_types(): bool {
return apply_filters( 'woocommerce_gla_enable_mcm', false ) === true;
Copy link
Member

Choose a reason for hiding this comment

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

💅 Since the filter name is woocommerce_gla_enable_mcm, I'd think is_multichannel_marketing_enabled() or just is_mcm_enabled() would be more suitable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I've added here: 1968a2f

@jorgemd24 jorgemd24 merged commit 6f291e9 into develop Jun 11, 2024
10 checks passed
@jorgemd24 jorgemd24 deleted the fix/fatal-error-when-fetching-marketing-campaings-for-mcm branch June 11, 2024 09:32
@puntope puntope mentioned this pull request Jun 18, 2024
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants