Skip to content

[NFR]: History Panel enhancements #32

Description

@niden

Additional enhancements after #23 is merged

The history cookie is set without a headers_sent()
(src/DebugBar/History/HistoryCookie.php:61, called from src/DebugBar/ResponseListener.php:73-78)

Sometimes output starts before beforeSendResponse. Examples: a deprecation notice in bootstrap with display_errors on, or echo/var_dump with useImplicitView(false) when output_buffering=0. output_buffering=0 is PHP's default when there is no php.ini, as in the official Docker images.

  • In that case, setcookie() gives an E_WARNING. The package's own Phalcon\Debug::listenLowSeverity() (src/Debug.php:191, 252-254) turns that warning into a RuntimeWarning exception inside the listener, so the user sees an error page instead of the page.

This happens on the first allowed response of each browser. Before this PR, the listener made no native header calls, and Phalcon's sendHeaders() skips silently in this case.

Options:
- (a) Return early in HistoryCookie::queue() when headers_sent() is true. Inject the check like $setCookie so tests can cover both paths.
- (b) Skip record() and queue() in ResponseListener when headers_sent() is true.

The query string is stored on disk without redaction
(src/DebugBar/ResponseListener.php:102, src/DebugBar/Collector/RequestCollector.php:65-68)

meta.uri stores the raw getURI(), for example /reset?token=abc. The Query section masks token, but the raw URI keeps the value. This goes around redact.mask.

We have three new places for the URI:
- metrics.uri
- the history list
- disk storage (the sidecar and the payload), kept for ttl_seconds (24 h by default).

Options:
- (a) Store only the path in the metadata (getURI(true), which exists on the v5 and v6 RequestInterface). In RequestCollector, send the parsed query through the Redactor and rebuild the URI for the URI row and metrics.uri.
- (b) Give the Redactor to the code that builds RequestMetadata, and store a redacted URI (parse_str → redact → http_build_query).

ResponseListener holds the history workflow
(ResponseListener.php:49-51, 61-78, 96-143; Provider.php:90-92, 170-199, 288-329)

The constructor now has 9 parameters. registerHistory() sets two nullable properties as a side effect, and then boot() checks both again.

Options:
- (a) Move the history steps into a new History\HistoryRecorder with isEndpointRequest() and record(...). ResponseListener then takes one ?HistoryRecorder, registerHistory() returns it, and the two properties go away.
- (b) Keep the listener as it is, but make registerHistory() return a small readonly object (history, endpoint, cookie) instead of setting properties.

The endpoint can hide application routes
(src/DebugBar/History/HistoryOptions.php:87-95)

history.url = '/' passes validation and sends the home page to the history controller. Any value that is the same as an application route replaces that route without a warning.

Options:
- (a) Reject / in validate().
- (b) Document that the endpoint wins over an application route with the same path.

CHANGELOG

Some behavior changes are not listed:
- meta.widgets is now sent, so the logger tab now reads "Logs" instead of "Logger", and panel types are declared instead of guessed.
- Time moved from a tab to an indicator.

Options:
- (a) Remove the "Fixed" block, undo the change on line 60, and add a "Changed" entry.
- (b) Merge the "Fixed" items into "Added", and add the "Changed" entry.

Long positional parameter lists in JS
(resources/assets/debugbar.js:110, 417)

renderIndicators takes 7 positional parameters and renderHistoryBrowser takes 6. The call sites are hard to read, and it is easy to put the arguments in the wrong order.

Options:
- (a) Pass one options object.
- (b) Split renderIndicators into renderMetricIndicators and renderRequestControl.

max browsers

It seems that max browsers reflects the max number of files PER browser and not how many browsers are open. Investigate if this is true or not. If it is, the name must change because any future developer will derive that this is an actual max browser windows vs files per browser.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

  • Status
    Active Sprint

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions