8000 [Routing][DX] Deprecated bundle:controller:action Syntax Is Only Triggered on First Request · Issue #27387 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing][DX] Deprecated bundle:controller:action Syntax Is Only Triggered on First Request #27387

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.

Sign up for GitHub

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

Jump to bottom
Closed
kiler129 opened this issue May 25, 2018 · 5 comments
Closed

[Routing][DX] Deprecated bundle:controller:action Syntax Is Only Triggered on First Request #27387

kiler129 opened this issue May 25, 2018 · 5 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Routing
Milestone
4.4

Comments

@kiler129
Copy link
Contributor

Description
In my large project I still have routes defined as bundle:controller:action, which is deprecated since #25910. @Tobion did a great job on that PR, but the deprecation message has very poor DX.

PR for that issue mentions that deprecation message is displayed only during first request (which may be e.g. redirect) and is hard to spot. I believe the message should be displayed every time route is matched.

WDYT?
@Tobion
Copy link
Contributor
Tobion commented May 25, 2018

but the deprecation message has very poor DX

Can you elaborate?

PR for that issue mentions that deprecation message is displayed only during first request

It happens when the container is built, not on the first request.

@stof
Copy link
Member
stof commented May 25, 2018

It happens when the container is built, not on the first request.

Actually, it happens during cache warmup, which makes things harder to spot.
Container-building deprecation are displayed by the profiler even on next requests since 3.4, but this is not the case for cache-warming deprecations.

@stof
Copy link
Member
stof commented May 25, 2018

I believe the message should be displayed every time route is matched.

the issue for that is that the cache warming is optimizing the router by resolving this notation to the class name during cache warmup. So when performing the matching itself in the request, the deprecation notation is not used.

@kiler129
Copy link
Contributor Author

@Tobion As stof said it's very hard to spot these messages and they are very useful. Also our code contains a lot of internal deprecations, so it's hard to expect from developers to pay attention to a message which only appears once.

@stof I looked into the code and indeed it replaces : with :: which makes checking after warming impossible. I also don't see a way to mark the route during the warming in any way...

My point is that any problem reported during compilation of container are persisted in profiler while problems with routes aren't. I showed this to few developers in the team and they were surprised such deprecation exists since they never seen it (in fact I myself found it not via profiler but while checking changelogs).

@stof
Copy link
Member
stof commented May 25, 2018

well, we would need a way to report cache warmup errors later, similar to what has been done for the container builder. That's something we can put on the roadmap for 4.2.

@stof stof added this to the next milestone May 25, 2018
@ScullWM ScullWM mentioned this issue May 30, 2018
Merged
@javiereguiluz javiereguiluz added Routing DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels May 31, 2018
nicolas-grekas added a commit that referenced this issue Jun 19, 2018
@nicolas-grekas
feature #27421 CacheWarmerAggregate handle deprecations logs (ScullWM)
4fde3bb 
This PR was merged into the 4.2-dev branch.

Discussion
----------

CacheWarmerAggregate handle deprecations logs

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #27387
| License       | MIT

Actually the Web Debug Toolbar warning you about the deprecation messages thrown during the container built (#21502).
Cache warmup can throw deprecated message without any persistance, it may cause issue like #27387
This PR reproduce the same job for the cache warmer, and so on, handle deprecated messages during the warmup of Twig, Translator, Validator, Security and all `kernel.cache_warmer` services.

Here are the point that may be improvable in this PR:

1.  Actually I've "duplicate" the callable used in the `set_error_handler` of the Kernel.
IMHO I think that Kernel and CacheWarmerAggregate have differents jobs and a trait may be a good solution to share this error handler setter without duplicating the code, but I'm a little bit lost about the repercussion of adding a Trait in the Kernel.

2. I've think about extending the `CacheWarmerAggregate` into a `DeprecatedLogHandlingCacheWarmerAggregate` to add the debug and containerClass argument, and declare it as the `cache_warmer` service only in debug mode (by declaring it in the DebugBundle/Resources/config/services.xml).

Commits
-------

f03b8bb CacheWarmerAggregate handle deprecations logs
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Routing
Projects
None yet
Milestone
4.4
Development

No branches or pull requests
5 participants
@javiereguiluz @nicolas-grekas @stof @Tobion @kiler129
0