Reduce frontend PHP bootstrap memory further - #1313
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughFrontend requests now defer selected admin, legacy, and notification initialization until related hooks execute. Notification provider access initializes the manager. Tests verify deferred hook registration and provider loading. ChangesDeferred initialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🟡 Moderate · up to This change defers notification and legacy handler loading to reduce frontend memory, but notification providers may be skipped for version updates or addon-defined events, and malformed deferred-hook configuration can cause runtime errors. The PR is not merge-ready until these bounded notification-path risks are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant FrontendRequest
participant WordPressHooks
participant NotificationManager
participant DeferredProvider
FrontendRequest->>NotificationManager: Register lazy boot
NotificationManager->>WordPressHooks: Register deferred boot callbacks
WordPressHooks->>NotificationManager: Fire notification-related hook
NotificationManager->>DeferredProvider: Initialize provider
DeferredProvider->>WordPressHooks: Register pum_alert_list callback
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
37934ea to
9bd5fa7
Compare
…ble hooks Per review feedback: the deferral mechanism now lives in the Notifications Manager itself instead of Plugin\Core, and the trigger hook list is extensible via the popup_maker/notifications/deferred_boot_hooks filter so Pro, Pro+, and legacy extensions can register their own. get_providers() remains the universal lazy-boot fallback, so unregistered consumers still work.
|
Addressed review feedback (via chat): the deferred-boot mechanism moved out of |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@classes/Plugin/Core.php`:
- Around line 384-386: Update the deferred-hook setup around
self::DEFERRED_NOTIFICATION_HOOKS to apply the
popup_maker/notifications/deferred_boot_hooks filter, retaining the default list
when the callback returns invalid data. Before add_filter(), discard non-string
hook names, then register init_notifications_on_demand for each valid hook.
- Around line 28-30: Remove popup_maker/update_version from
DEFERRED_NOTIFICATION_HOOKS and ensure its dispatch occurs after init_services()
registers the init callback, or preserve the upgrade state and dispatch it from
the init flow so frontend notification providers receive the event.
In `@tests/php/tests/Notification_Manager_Loader_Test.php`:
- Around line 22-30: Update the test setup around
PUM_Test_Deferred_Notification_Provider and Manager so it uses the global
notifications service instead of the local $loader that directly calls
Manager::init(). Apply the pum_alert_list filter, assert the expected alert
exists, and then verify the provider is booted, ensuring the test exercises
Core’s lazy-boot registration and init_notifications_on_demand() path.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e9e923dd-7342-41dd-a42c-a61eb7b9c87a
📒 Files selected for processing (9)
classes/Admin.phpclasses/Plugin/Core.phpclasses/Services/Notifications/Manager.phpclasses/Services/Notifications/Provider.phpincludes/legacy/class-popup-maker.phptests/php/fixtures/class-pum-test-deferred-notification-provider.phptests/php/tests/Notification_Manager_Loader_Test.phptests/php/tests/PUM_Admin_Loader_Test.phptests/php/tests/PUM_Deferred_Hooks_Test.php
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bd5fa7a2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| foreach ( self::DEFERRED_NOTIFICATION_HOOKS as $hook ) { | ||
| add_filter( $hook, [ $this, 'init_notifications_on_demand' ], PHP_INT_MIN ); | ||
| } |
There was a problem hiding this comment.
On frontend and REST requests, providers are now initialized only when one of the hard-coded DEFERRED_NOTIFICATION_HOOKS fires. The provider contract explicitly permits providers to register callbacks for “other plugin events,” so an addon provider listening to an event outside that list—for example, a form or commerce submission hook—will never be initialized before its event and will silently miss it; previously every provider was initialized on init. Preserve automatic initialization for extension providers or provide a mechanism that does not require core to know every provider-specific hook.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Covered by 9f7d898b + fd07bc77 (pushed after your review): (1) hook list is extensible via popup_maker/notifications/deferred_boot_hooks, (2) get_providers() lazy-boots as a universal fallback for any consumer, (3) hooks that fired before init now boot the manager immediately. Extension providers registered via popup_maker/notification_providers resolve at boot regardless of which path triggered it.
An upgrade dispatches popup_maker/update_version during plugins_loaded, before init@5 registers the lazy boot filters. Check did_action() and did_filter() for each deferred hook and boot the manager immediately if the event already fired this request. Adds a regression test plus a Core-wiring assertion through the booted plugin.
1 participant
Stack
Builds on #1302 and can be reviewed/skipped as a separate layer. Retarget to
developafter #1302 merges.TL;DR
Removes another 124,616 B (-28.4%) from Popup Maker's retained frontend footprint and 131,344 B (-41.6%) from its peak footprint versus #1302. Cumulatively, the first two PRs reduce retained Popup Maker bootstrap overhead from about 1.45 MB on
developto 0.314 MB, or roughly 78%.Measurement definitions
time_starttransfer. It was not benchmarked for this PR.Issues and gains
Legacy handler classes are compiled before their hooks run
Privacy, alerts, telemetry, and the popup/theme editor handlers were loaded on every frontend request just to register WordPress callbacks. The bootstrap now registers the same class-string callbacks without autoloading their implementations; frontend programmatic saves and the full eager admin bootstrap remain intact.
Isolated gain: 25,384 B retained, 32,112 B peak, 5 files, and 5 classes.
Notification providers boot when no notification API is used
The notification manager constructed
WhatsNewandFeatureAnnouncementsduringinit, even when no alert, dismissal, save, option-change, or plugin lifecycle event occurred. Priority-PHP_INT_MINloaders now boot providers on the relevant event early enough to participate in that same hook; admin requests and explicitget_providers()calls stay eager.Isolated gain: 4 files and 3 classes. Its direct byte delta is allocator-masked (~0.5 KB), but combining it with the handler deferral releases another PHP allocator slab.
Popup Maker footprint: before / after
Matched core-only WordPress bootstrap, theme skipped, other active plugins skipped, five stable runs.
Secondary whole-process measurements
PHP's allocator makes the two ablations non-additive; the combined process measurement is the number that matters.
Functional verification
pum_alert_listfilter execute during that same iteration.Validation
composer run tests: 910 tests, 1,918 assertions; 29 skipped.Summary by CodeRabbit
Performance
Bug Fixes
Tests