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

Log Exception if an asset is enqueued before being registered #2576

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

martynmjones
Copy link
Contributor

@martynmjones martynmjones commented Sep 3, 2024

Changes proposed in this Pull Request:

Closes #1659

Adjusts asset enqueuing to catch InvalidAsset exceptions thrown when an asset is enqueued before being registered.

Detailed test instructions:

  1. Without checking out this branch, install and activate WooCommerce Print Invoices and Packing Lists on a site that has GLA setup complete
  2. Install and activate WP Consent API
  3. Make sure Google Analytics for WooCommerce is not active on the site
  4. Go to WooCommerce > Settings > Invoices/Packing Lists
  5. Click on Customize under Template Customizer
  6. See the fatal error triggered by the uncaught InvalidAsset exception
  7. Checkout fix/1659-asset-enqueued-before-registered
  8. Repeat step 3
  9. Confirm no error is displayed
  10. Go to WooCommerce > Status > Logs
  11. Confirm the error was logged in the google-listings-and-ads-... log file

Changelog entry

Fix - Log exceptions triggered by assets being enqueued before being registered

@martynmjones martynmjones self-assigned this Sep 3, 2024
@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 Sep 3, 2024
Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.2%. Comparing base (93b4180) to head (0baf754).
Report is 27 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             develop   #2576     +/-   ##
===========================================
+ Coverage       64.5%   65.2%   +0.6%     
- Complexity      4591    4592      +1     
===========================================
  Files            796     475    -321     
  Lines          22992   17909   -5083     
  Branches        1234       0   -1234     
===========================================
- Hits           14837   11671   -3166     
+ Misses          7982    6238   -1744     
+ Partials         173       0    -173     
Flag Coverage Δ
js-unit-tests ?
php-unit-tests 65.2% <100.0%> (+0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/Assets/BaseAsset.php 75.7% <100.0%> (+18.4%) ⬆️

... and 324 files with indirect coverage changes

@martynmjones martynmjones marked this pull request as ready for review September 3, 2024 16:49
@martynmjones martynmjones requested a review from a team September 3, 2024 16:50
Copy link
Contributor

@jorgemd24 jorgemd24 left a comment

Choose a reason for hiding this comment

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

Hi @martynmjones, thanks for working on this! However, I wasn't able to reproduce the issue. I followed the steps but didn't get the fatal error. On my local machine I see the following when I go to Customize:

  1. The plugin_loaded hook is called, which registers the GlobalSiteTag Assets:

$this->register_assets();

  1. The template_redirect hook is called, and wp_footer is triggered in the PIP plugin at: wp-content/plugins/woocommerce-pip/src/admin/class-wc-pip-customizer.php:880.

The PIP version that I am using is: 3.13.6

What am I doing differently?

@martynmjones
Copy link
Contributor Author

What am I doing differently?

Apologies @jorgemd24, for the error to be triggered in the example I gave, we need GLA to enqueue gla-wp-consent-api so these conditions need to be satisfied on the test site.

I've updated the test instructions to include installing and activating WP Consent API and making sure Google Analytics for WooCommerce is not active. You should then see the fatal error in the customizer.

Apologies again for the trouble!

Copy link
Contributor

@jorgemd24 jorgemd24 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 the explanation! I was able to reproduce the issue, and I see that this PR catch the fatal error, so it looks good to me!

@martynmjones martynmjones merged commit bf5c219 into develop Sep 5, 2024
14 checks passed
@martynmjones martynmjones deleted the fix/1659-asset-enqueued-before-registered branch September 5, 2024 10:42
@martynmjones martynmjones mentioned this pull request Sep 5, 2024
22 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.

The asset "gla-gtag-events" was not registered before it was enqueued.
2 participants