Fixed: add missing $in_footer parameter to wp_enqueue/register_script calls#308
Conversation
fix: add missing $in_footer parameter to wp_enqueue/register_script c…
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ChangesScript Loading Optimization
CI Cache Update
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Assets.php`:
- Line 43: Two integration scripts (plausible-affiliate-links and
plausible-form-submit-integration) declare 'plausible-analytics' as a dependency
with in_footer => true which forces the main plausible-analytics script to the
footer and breaks inline DOMContentLoaded listeners; fix by either removing
in_footer => true from those integrations so they enqueue in the header (keep
plausible-analytics header behavior controlled by the
plausible_load_js_in_footer filter), or update the inline handlers (the
DOMContentLoaded listeners registered near the inline event handler blocks and
in the code that depends on plausible-analytics) to run immediately if
document.readyState is already interactive/complete (or use an immediate
execution guard) so they work when scripts load after DOMContentLoaded.
- Line 43: The test asserts the affiliate links script is printed during wp_head
but Assets enqueues 'plausible-affiliate-links' with in_footer => true, so
update the integration test (AssetsTest) to trigger and capture
do_action('wp_footer') instead of (or in addition to) do_action('wp_head') and
assert that the output contains plausible-affiliate-links.js and the
plausibleAffiliateLinks const; locate the enqueue call for
'plausible-affiliate-links' in the Assets class and the assertion/do_action
calls in AssetsTest to make the matching change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c6b5425-4836-4256-9fe1-8ff08fef3c92
📒 Files selected for processing (3)
src/Assets.phpsrc/Integrations/FormSubmit.phpsrc/Integrations/WooCommerce.php
| PLAUSIBLE_ANALYTICS_PLUGIN_URL . 'assets/dist/js/plausible-affiliate-links.js', | ||
| [ 'plausible-analytics' ], | ||
| filemtime( PLAUSIBLE_ANALYTICS_PLUGIN_DIR . 'assets/dist/js/plausible-affiliate-links.js' ), | ||
| [ 'in_footer' => true ], |
There was a problem hiding this comment.
Cross-file root cause: Two integration scripts force the main analytics script to footer.
Both plausible-affiliate-links (src/Assets.php line 43) and plausible-form-submit-integration (src/Integrations/FormSubmit.php lines 61-62) declare 'plausible-analytics' as a dependency and set in_footer => true. WordPress dependency resolution moves the parent 'plausible-analytics' script to the footer when any dependent child is marked for footer placement.
This creates a timing hazard: the main script at src/Assets.php:104 defaults to header placement via the filter plausible_load_js_in_footer (default false), and several inline event handlers at lines 77, 172, and 204 register DOMContentLoaded listeners. When scripts execute in the footer (after DOM parsing), DOMContentLoaded has already fired, so these listeners never run.
Recommended fix: Either (1) keep integration scripts in the header to preserve current behavior, or (2) replace DOMContentLoaded listeners with immediate execution guards or alternative ready-state checks that work when scripts load after DOM is ready.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Assets.php` at line 43, Two integration scripts
(plausible-affiliate-links and plausible-form-submit-integration) declare
'plausible-analytics' as a dependency with in_footer => true which forces the
main plausible-analytics script to the footer and breaks inline DOMContentLoaded
listeners; fix by either removing in_footer => true from those integrations so
they enqueue in the header (keep plausible-analytics header behavior controlled
by the plausible_load_js_in_footer filter), or update the inline handlers (the
DOMContentLoaded listeners registered near the inline event handler blocks and
in the code that depends on plausible-analytics) to run immediately if
document.readyState is already interactive/complete (or use an immediate
execution guard) so they work when scripts load after DOMContentLoaded.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check test expectations for script placement
echo "=== Checking test expectations for affiliate links script ==="
rg -n "plausible-affiliate-links" tests/ -B 3 -A 3
echo -e "\n=== Checking for wp_head vs wp_footer in tests ==="
rg -n "do_action.*wp_(head|footer)" tests/ -B 2 -A 2Repository: plausible/wordpress
Length of output: 1028
Fix integration test to assert wp_footer output for in_footer enqueued affiliate links script.
src/Assets.php enqueues plausible-affiliate-links with ['in_footer' => true], but tests/integration/AssetsTest.php only calls do_action( 'wp_head' ) and asserts the output contains plausible-affiliate-links.js (and const plausibleAffiliateLinks). With in_footer => true, the script should be emitted during wp_footer, so update the test to trigger/capture do_action( 'wp_footer' ) (or adjust the assertions to match the new placement).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Assets.php` at line 43, The test asserts the affiliate links script is
printed during wp_head but Assets enqueues 'plausible-affiliate-links' with
in_footer => true, so update the integration test (AssetsTest) to trigger and
capture do_action('wp_footer') instead of (or in addition to)
do_action('wp_head') and assert that the output contains
plausible-affiliate-links.js and the plausibleAffiliateLinks const; locate the
enqueue call for 'plausible-affiliate-links' in the Assets class and the
assertion/do_action calls in AssetsTest to make the matching change.
There was a problem hiding this comment.
You're wrong there. Maybe that has changed in recent WP versions, but it's perfectly possible now to enqueue dependend script in the footer while the parent script remains in head.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WordPress wp_enqueue_script in_footer dependency resolution: can a child script be in footer while parent script remains in header? WordPress version change
Length of output: 18679
@Dan0sz, you're absolutely right, and I apologize for the incorrect analysis. My understanding of WordPress dependency resolution was backwards.
WordPress promotes dependencies upward (from footer → header) to ensure they load before the script that depends on them — not downward. So when plausible-affiliate-links (child, in_footer => true) lists plausible-analytics as a dependency, WordPress leaves plausible-analytics in the header where it was registered and simply emits the child script in the footer afterward. The parent loads first (in <head>), the child loads later (in footer) — perfectly valid ordering.
My concern about DOMContentLoaded listeners breaking was also unfounded: those inline scripts are attached to plausible-analytics which remains in the header and fires normally.
Would you like me to resolve my two comments on this PR since they were based on an incorrect assumption?
✏️ Learnings added
Learnt from: Dan0sz
Repo: plausible/wordpress PR: 308
File: src/Assets.php:43-43
Timestamp: 2026-06-10T12:47:59.014Z
Learning: In WordPress, when a child script sets `in_footer => true` but its dependency (parent) is registered for the header, WordPress does NOT move the parent to the footer. WordPress dependency resolution only promotes dependencies upward (footer → header) to ensure they load before the dependent script. So a footer-placed child script can safely depend on a header-placed parent script — the parent stays in the header and the child loads in the footer afterward. Flagging this pattern as a "timing hazard" in the plausible/wordpress plugin (or any WordPress plugin) is incorrect.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Dan0sz
Repo: plausible/wordpress PR: 289
File: src/Compatibility.php:271-271
Timestamp: 2026-03-25T12:57:52.477Z
Learning: In the plausible/wordpress plugin, it is acceptable to use `str_contains()`, `str_starts_with()`, and `str_ends_with()` even if this project declares a minimum PHP version below 8.0. The plugin’s WordPress requirement (WordPress 5.9+) provides polyfills for these functions on PHP < 8.0, so review should not flag these calls as incompatible with older PHP versions as long as the WordPress 5.9+ assumption remains true.
There was a problem hiding this comment.
Yes, please. You were right about the test breaking, though. It should be fixed in #0cc6432.
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Summary by CodeRabbit