-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[ErrorHandler][FrameworkBundle] Add support for selecting the appropriate error renderer based on the APP_RUNTIME_MODE
#60033
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
Conversation
|
Now after updating some related tests, I'm concerned that this might break some functional tests out there (after upgrading to 7.3) for example: testing HTML error pages using Should we consider this a BC break? |
c281ec6 to
bb5934f
Compare
|
Hi, Thanks, it looks good to me at first glance. |
Is there a way to opt-in for this feature (e.g. a dedicated env var)? |
This small change restores the previous behavior. We could document this adjustment for situations where you specifically want to test the actual HTML error page: - ->alias('error_renderer.default', 'error_handler.error_renderer.default')
+ ->alias('error_renderer.default', 'error_renderer.html')Most functional/end-to-end tests have a dedicated config file. That could be better than using an env var, since you can enable or disable this behavior case by case. |
|
Or we could add a new framework option to opt-in for this behavior, include a deprecation notice to make it the default in 8.0, and update the recipe file to set it as the default for new projects. What do you think? |
|
Sounds good 👍 |
|
support 👍 |
src/Symfony/Component/ErrorHandler/ErrorRenderer/SapiErrorRendererSelector.php
Outdated
Show resolved
Hide resolved
|
Still pending on my side to add the deprecation notice to make the new option defaults to |
|
Test failure on php_errors_log_levels.yml is a false-positive. @symfony/mergers This PR is ready for 7.4 |
src/Symfony/Bundle/FrameworkBundle/ErrorHandler/ErrorRenderer/SapiErrorRendererSelector.php
Outdated
Show resolved
Hide resolved
nicolas-grekas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can and should wait for 8.1
Also because with my latest suggestion, the naming isn't appropriate anymore, and I'm not super happy with adding a new option to everybody's config once again in 7.x.
Also, I'm wondering: couldn't we remove the option? I read that web test cases are the issue. But shouldn't web test cases run in "web" mode?
|
Now with the last suggestion I think the option can be removed as ppl will have the APP_RUNTIME_MODE to switch when they need it. I will do the changes so we can merge this in 7.4, is it ok for you @nicolas-grekas ? |
SapiErrorRendererSelector for context-based error renderingAPP_RUNTIME_MODE
a43365f to
ef0f23e
Compare
...mfony/Bundle/FrameworkBundle/ErrorHandler/ErrorRenderer/RuntimeModeErrorRendererSelector.php
Outdated
Show resolved
Hide resolved
561e35a to
4920c79
Compare
src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php
Outdated
Show resolved
Hide resolved
…iate error renderer based on the `APP_RUNTIME_MODE`
ede344c to
692ce37
Compare
|
Thank you @yceruto. |
… of inline service (yceruto) This PR was merged into the 7.4 branch. Discussion ---------- Fix argument syntax for callable and lazy initialization of inline service | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT Fix tests from #60033 Commits ------- 0f6b9eb Fix argument syntax for callable and lazy initialization of inline service
|
As anticipated by @yceruto this breaks some of our functional tests. It can be bypassed by passing |
Could you please share the whole code to set Update: I put it in <php>
<env name="APP_RUNTIME_MODE" value="web=1"/>
</php> |
Alternative approach to #58456 (same goal), using a factory selector to pick the correct error renderer based on the runtime mode (
APP_RUNTIME_MODE). One advantage of this solution is that it works even ifTwigBundleis not installed as the selector/decider doesn't depend on it.