Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Revert "bug #33092 [DependencyInjection] Improve an exception message"#33108
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabpot commentedAug 10, 2019
Let's not revert before we understand. |
ro0NL commentedAug 11, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@fabpot a stacktrace from the original issue would be helpful :) there are a few possible known suspects:https://github.com/symfony/symfony/search?l=PHP&q=%22Class+%25s+used+for+service+%25s+cannot+be+found.%22 the rationale isnt 100% clear to me, i.e. why exactly didnt we detect Now, some passes will benefit from#33111 when running "before removing" (e.g. So we may have somewhat of a chicken/egg situation, nevertheless the behavior change is real. I guess the user may process such service IDs themselves (so we are as preservative as possible). For reproducible builds we don't involve class_exists (correct me if im wrong), as such we would make a big assumption on service IDs. Hence we provide an error as late as possible. |
nicolas-grekas commentedAug 11, 2019
I can answer that: because we do not want any normalization logic around ids. We got rid of any when making them case sensitive and that reduced the complexity by a nice margin. We do not want to introduce it again in a different flavor. |
ro0NL commentedAug 11, 2019
sorry, ive updated my comment :) basically, why didnt we do#33092 when introducing ResolveClassPass |
ro0NL commentedAug 11, 2019
the discussion im recalling is#28006, same topic really 😅 |
…ption message" (nicolas-grekas)This PR was merged into the 4.3 branch.Discussion----------Revert "bug#33092 [DependencyInjection] Improve an exception message"| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -As reminded by@ro0NL in#33092 (comment), it looks like we forgot that `CheckDefinitionValidityPass` already checks and suggests for leading slashes.Why didn't you get the exception from `CheckDefinitionValidityPass`@fabpot?Commits-------ed590ca Revert "bug#33092 [DependencyInjection] Improve an exception message (fabpot)"
As reminded by@ro0NL in#33092 (comment), it looks like we forgot that
CheckDefinitionValidityPassalready checks and suggests for leading slashes.Why didn't you get the exception from
CheckDefinitionValidityPass@fabpot?