Conversation
|
@Alistar84 At some point please rebase from master. I put a fix in for the warning emitted in the CI that makes this CI red. Thanks |
d26cba8 to
603fa1c
Compare
|
Rebased onto the latest master. Thanks for the fix! |
|
@Alistar84 Can we look in this one when you can? You will need to merge master. I am guessing the coverage dropped for this one and also I introduced new php-cs-fixer rules so run it when you can over here. |
Assisted-by: Codex
Assisted-by: Codex
Assisted-by: Codex
Harden session storage, request handling, and asynchronous history controls while preserving legacy entries and full test coverage. Assisted-by: Codex
Assisted-by: Codex
Assisted-by: Codex
603fa1c to
11b8040
Compare
|
Hi @niden, I've updated the PR following your comments about merging the latest
Co-authorship disclosure: these changes were co-authored with Codex (OpenAI), which assisted with implementation, tests, and review. |
|
@Alistar84 Thank you for this. I will check it out later tonight. It looks good at first glance but I have noticed a couple of things I need to check locally first before posting. More to come later on. |
niden
left a comment
There was a problem hiding this comment.
Some comments listed inline.
This is really good work and the main mechanics are in place. Have a look at the comments and share your thoughts on them.
| return; | ||
| } | ||
|
|
||
| $uri = $this->request->getURI(); |
There was a problem hiding this comment.
We have an issue here. As is, any app that lives in a subfolder will not work i.e. /app1/_debugbar..... You already have the names of the routes from the provider.
There was a problem hiding this comment.
Good catch. The history URL now includes the application base URI, when available, so /app1/_debugbar/open works too. The same endpoint object is also used to recognize history requests.
| if ($historyEnabled && $this->isCollectorEnabled(MemoryCollector::NAME)) { | ||
| $collectors[] = new MemoryCollector(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Are we sure we want to do that? The memory collector is tied to the history being enabled.
There was a problem hiding this comment.
Adding more relating to the debugbar.js and payload.meta.widgets.
Right now we have the same implementation three times in two languages. One will have to do shotgun surgery to just add one more provider. More speficially:
- MemoryCollector is built when history is enabled
- time and memory are hidden when history is present (comment above)
- the indicators come from
data.time.badgeand thedata.memory.panel.
As a result, a user that sets memory = false in the collectors with history on, does not see the memory (again comment above) and cannot find out why. If we also set history = false the collector/routes etc. still remain active. If a third indicator is required later on, it will need a new collector, a name in the JS skip list etc. In short the change has to be propagated in many places.
Let us take advantage of the meta.widgets node. Have PHP be the source of truth that will drive what is going on in JS (at least for this PR). Do not expand the logic in other panels, that is out of scope. This way history is driven only by one contract which is held in the PHP side.
This "PHP contract" has been on my mind as a todo, but since you brought this new tab over, might as well start it here and then we can expand it to the remaining panels.
There was a problem hiding this comment.
Note: if you don't want to do it, that is fine, I can do that in the next PR. Just keeping this comment here as a documentation for my future self :)
There was a problem hiding this comment.
Agreed. History no longer enables the Memory collector automatically. Memory now follows only the collectors.memory setting.
There was a problem hiding this comment.
Implemented. PHP now defines the History widget and its indicators. JavaScript no longer has hardcoded Time/Memory skip lists or data paths. If a collector is disabled, its indicator simply is not rendered.
There was a problem hiding this comment.
Thanks! I decided to include it in this PR since it removes the coupling introduced by History and keeps PHP as the source of truth for the new indicators. I kept the broader refactoring of the existing panels out of scope.
| if ( | ||
| name === 'history' | ||
| || (historyPanel && (name === 'time' || name === 'memory')) | ||
| ) { |
There was a problem hiding this comment.
This one makes the time and memory panels skipped whenever the historyPanel is on. As such only their badges survive. Turning on history, removes access to those measurements. I would keep both tabs next to their indicators or make the indicator open the matching panel.
There was a problem hiding this comment.
Fixed. Time and Memory remain available as normal tabs when History is enabled, and their values are also shown in the indicators on the right.
My initial preference was to hide those tabs while History is enabled, since their main values are already visible in the indicators and this keeps the bar more compact. However, I understand your point that the panels provide additional details and should remain accessible, so I followed your suggestion and kept both tabs. We can always revisit this later, possibly as an optional UI setting, if there is interest.
| * stored_at: string | ||
| * } | ||
| */ | ||
| final class FilesystemHistory |
There was a problem hiding this comment.
Since this is final (agree), I would like to see an interface here. This way we do not rely on the concrete class for other consumers of this one. The benefit is that we are defining the contract that each of those consumers uses and we can also create a Fake for our tests without breaking the final.
There was a problem hiding this comment.
Done. I added a History interface and updated the consumers to depend on it instead of the concrete FilesystemHistory class. This also makes using fakes in tests easier.
| 'payload' => $payload, | ||
| ]; | ||
|
|
||
| $flags = JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE; |
There was a problem hiding this comment.
A note here JSON_PRETTY_PRINT nearly doubles the storage of the JSON payload. If there is a need to view the file say in a browser or text editor fine, but the JSON.parse can handle it without it. Just a thought.
There was a problem hiding this comment.
Makes sense. I removed JSON_PRETTY_PRINT from both the stored payload and metadata.
|
|
||
| var memory = data.memory || {}; | ||
| var memoryPanel = memory.panel || {}; | ||
| var currentMemory = memoryPanel['Current usage']; |
There was a problem hiding this comment.
A typo or a change in the display string will break this. Can we use some sort of a key here?
There was a problem hiding this comment.
I see the RequestCollector does the same so that will have to be a refactoring after this PR.
There was a problem hiding this comment.
Good point. The indicator no longer depends on the display label Current usage. Memory now exposes a stable metrics.current_usage key, which is referenced by the PHP widget metadata.
There was a problem hiding this comment.
Agreed. I left RequestCollector unchanged for now and kept this PR focused on the new History contract. We can handle the broader refactoring separately.
| @@ -243,27 +591,78 @@ | |||
| var widgets = (payload.meta && payload.meta.widgets) || {}; | |||
There was a problem hiding this comment.
It makes more sense to utilize this to transfer data regarding the collectors. See below in Provider.
There was a problem hiding this comment.
Done. The indicator definitions now come from payload.meta.widgets. JavaScript just reads the collector, icon, label, and semantic path provided by PHP.
| * There is no DI service to register and no container-specific wiring - the | ||
| * app hands over its container and event bus. | ||
| * When request history is enabled it additionally registers an internal route | ||
| * and two private DI services used by its controller. |
There was a problem hiding this comment.
This kinda contradicts the statement above. The purpose is to keep this decoupled as much as possible. However further down when registering the history we are creating routes in the router service, then read those services back, treating them as transports.
Now boot() depends on the router of the consumer and is not isolated, a coupling that can certainly break in many applications because of the way people register their routes is not universal. A /:controller/:action route which is very common will yield different results in an app that has the debugbar on.
IMHO the best way is to keep the endpoint within the package. There is no real reason to keep the history in a separate service. Injecting the FilesystemHistory and AccessGate to a class gives you exactly what you need, and all that will be required is registering the controller in the DI container. A lot of the code is removed that way.
There was a problem hiding this comment.
Reworked following your suggestion. The package no longer adds or reads application routes. It registers the History controller in the DI and selects it through a package-owned endpoint listener, leaving the application router untouched.
|
|
||
| $storedAt = new DateTimeImmutable('now', new DateTimeZone('UTC')); | ||
| $requestedAt = $request->requestedAt ?? $storedAt; | ||
| $id = $storedAt->format('YmdHis-u-') . bin2hex(random_bytes(4)); |
There was a problem hiding this comment.
This stores the payload and it is read as is (only check is if it is an array). The payload might become stale and also a future you or me might introduce new elements to it. A safer approach here is to make this very specific. A value object that creates the $entry with a toArray() but also incorporates a "version" of sorts in it, can be the shape we want in the JS side. This way future upgrades of this (composer update) will only read the correct content. If I upgrade and the shape changed for whatever reason, then JS will match only the new version and the old (potentially stale data) will be discarded.
There was a problem hiding this comment.
Done. Stored requests now go through a versioned HistoryEntry. Both PHP and JavaScript reject incompatible versions, and PHP also validates the metadata and payload shape before returning an entry.
| int $maxRequests = 100, | ||
| int $ttlSeconds = 86400 | ||
| ) { | ||
| $this->path = rtrim('' !== $path ? $path : sys_get_temp_dir() . '/phalcon-debugbar', '/\\'); |
There was a problem hiding this comment.
We cannot rely on sys_get_temp_dir() or allow content to be written in it. Call me old fashioned or paranoid but there have been many exploits in the past that came from /tmp.
Let us make this mandatory i.e. a folder that you must specify as a user i.e. where is your history written. Provision of course for read/write of the web server user.
There was a problem hiding this comment.
Agreed. The temporary-directory fallback is gone. history.path is now required whenever History is enabled.
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
|
Thanks for the detailed review. I have gone through all the comments and pushed the related changes. The main updates are the package-owned History endpoint, the History and filesystem contracts, versioned stored entries, PHP-driven widget metadata, independent collector settings, and the required storage path. Local checks are green: 297 PHP tests, 18 JavaScript tests, PHPStan, PHPCS, and PHP CS Fixer across all 164 files. Both new commits also include the requested Co-authored-by: Codex codex@openai.com trailer. |
Hello!
In raising this pull request, I confirm the following:
Small description of change:
This PR adds an optional, session-scoped request history system to the debug bar and integrates it with compact request metrics in the bottom bar.
It resolves #21, which describes the current limitation where the bar can only inspect the request that rendered the current page. Completed AJAX requests, redirects, and earlier requests can now be inspected without navigating away from or reloading the host page.
Closes #21.
History browser
When history is enabled, each stored entry contains the collected debug-bar payload and request metadata such as:
The rightmost request control combines a search icon with the HTTP method and URI and replaces a dedicated
Historytab. It identifies the current request when the page first loads and updates dynamically when a stored request is selected.Clicking the request control closes any open collector panel and opens the history browser. Opening a collector panel closes history, so only one panel is visible at a time. The browser provides refresh and clear controls. Selecting a request closes history and replaces the complete bar payload, including its tabs, request time, current memory usage, method, and URI.
The UI also adds a memory collector for current and peak PHP memory usage. Request time and current memory usage are shown as compact right-side indicators instead of dedicated
TimeandMemorytabs.Storage, retention, and performance
History storage is disabled by default and must be enabled explicitly through configuration.
Stored requests are:
Each payload has a small metadata sidecar, so listing requests does not read every full collector payload. Legacy entries without sidecars remain readable. History reads clean expired entries only for the active session; rate-limited garbage collection removes expired entries, abandoned temporary files, and empty directories across sessions.
The storage path defaults to the system temporary directory and can be configured outside the application document root.
Internal endpoint
When history is enabled, the provider automatically registers an internal controller for:
GET /_debugbar/open- list stored request metadataGET /_debugbar/open?id=<request-id>- load a stored debug-bar payloadDELETE /_debugbar/open- clear the current session's historyThe endpoint reuses the debug bar access gate, returns private non-cacheable responses, validates request IDs, and excludes its own requests from history.
Backward compatibility
The feature is opt-in. Existing applications retain the current behavior when
history.enabledis not enabled.Tests
The PR includes PHP and JavaScript tests covering:
Thanks