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

Revert "bug #33092 [DependencyInjection] Improve an exception message" #33108

Merged
merged 1 commit into from Aug 20, 2019

Conversation

@nicolas-grekas
Copy link
Member

commented Aug 10, 2019

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?

@fabpot

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Let's not revert before we understand.

Revert "bug #33092 [DependencyInjection] Improve an exception message…
… (fabpot)"

This reverts commit 2f2d1aa, reversing
changes made to 07cf927.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-revert branch from f321d35 to ed590ca Aug 10, 2019

@nicolas-grekas nicolas-grekas changed the title Revert "bug #33092 [DependencyInjection] Improve an exception message… Revert "bug #33092 [DependencyInjection] Improve an exception message" Aug 11, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

@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 \Foo\Bar vs. Foo\Bar at an early stage? We had some discussion about it when introducing ResolveClassPass i believe.

Now, some passes will benefit from #33111 when running "before removing" (e.g. AddConsoleCommandPass) whereas others will benefit from #33092 (e.g. RegisterEnvVarProcessorsPass).

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

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2019

why we dont consider \Foo\Bar vs. Foo\Bar equal. We had some discussion about it when introducing ResolveClassPass i believe

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

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

sorry, ive updated my comment :) basically, why didnt we do #33092 when introducing ResolveClassPass

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

the discussion im recalling is #28006, same topic really 😅

nicolas-grekas added a commit that referenced this pull request Aug 20, 2019

minor #33108 Revert "bug #33092 [DependencyInjection] Improve an exce…
…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)"

@nicolas-grekas nicolas-grekas merged commit ed590ca into symfony:4.3 Aug 20, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.