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

[TwigBunde] [WebProfilerBundle] Exclude Some Vendors From Being Hidden #31103

Conversation

Projects
None yet
5 participants
@Simperfit
Copy link
Contributor

Simperfit commented Apr 13, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27409
License MIT
Doc PR todo when implementation is validated

This implementation adds a new parameter to allow excluded vendor.

@Simperfit Simperfit force-pushed the Simperfit:feature/exclude-private-vendors-from-being-excluded branch from 2665ffc to d14d3ed Apr 13, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 14, 2019

@Simperfit Simperfit force-pushed the Simperfit:feature/exclude-private-vendors-from-being-excluded branch from d14d3ed to 5c93df4 Apr 15, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

Simperfit commented Apr 15, 2019

Thanks @OskarStark for the review !

@@ -54,6 +54,7 @@ public function load(array $configs, ContainerBuilder $container)
$container->setParameter('web_profiler.debug_toolbar.intercept_redirects', $config['intercept_redirects']);
$container->setParameter('web_profiler.debug_toolbar.mode', $config['toolbar'] ? WebDebugToolbarListener::ENABLED : WebDebugToolbarListener::DISABLED);
}
$container->setParameter('web_profiler.debug_toolbar.private_vendors', $config['private_vendors']);

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 15, 2019

Contributor

why not inject the argument directly?

@@ -22,7 +22,7 @@
<div id="trace-html-{{ index }}" class="sf-toggle-content">
{% set _is_first_user_code = true %}
{% for i, trace in exception.trace %}
{% set _is_vendor_trace = trace.file is not empty and ('/vendor/' in trace.file or '/var/cache/' in trace.file) %}
{% set _is_vendor_trace = trace.file is not empty and ('/vendor/' in trace.file or '/var/cache/' in trace.file or trace.file not in private_vendors) %}

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 15, 2019

Contributor

trace.file not in private_vendors

this requires the abs. file path to be configured? 🤔 shouldnt we test each private vendor separate?

@@ -75,6 +77,7 @@ public function showAction($token)
'status_text' => Response::$statusTexts[$code],
'exception' => $exception,
'logger' => null,
'private_vendors' => $this->privateVendors,

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 15, 2019

Contributor

twig-bundle dependency needs a bump to 4.3

@ro0NL
Copy link
Contributor

ro0NL left a comment

last but not least, does this enforce the same behavior between the exception page in the profiler and outside? should we find a different configuration hook?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 15, 2019

The linked issue suggests:

When vendor part of PSR-4 namespace matches current project namespace code should be treated as belonging to the project..

it would be great to try implementing this instead.

As far as this PR is concerned, I don't think adding an option for this will work. Almost nobody has time to discover, learn, configure, maintain such a feature that "only" enhances DX marginally.

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

Simperfit commented Apr 17, 2019

I've taken @nicolas-grekas comments in account, just taking some time to think on how to do this, will repusha new PR lately in the week.

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

Simperfit commented Apr 17, 2019

Status: Needs Work

@Simperfit Simperfit closed this Apr 17, 2019

@Simperfit Simperfit deleted the Simperfit:feature/exclude-private-vendors-from-being-excluded branch Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.