Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
nicolas-grekas merged 1 commit intosymfony:4.3fromnicolas-grekas:di-revert
Aug 20, 2019

Conversation

@nicolas-grekas
Copy link
Member

QA
Branch?4.3
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

As reminded by@ro0NL in#33092 (comment), it looks like we forgot thatCheckDefinitionValidityPass already checks and suggests for leading slashes.

Why didn't you get the exception fromCheckDefinitionValidityPass@fabpot?

@fabpot
Copy link
Member

Let's not revert before we understand.

nicolas-grekas reacted with thumbs up emoji

…message (fabpot)"This reverts commit2f2d1aa, reversingchanges made to07cf927.
@nicolas-grekasnicolas-grekas changed the titleRevert "bug #33092 [DependencyInjection] Improve an exception message…Revert "bug #33092 [DependencyInjection] Improve an exception message"Aug 11, 2019
@ro0NL
Copy link
Contributor

ro0NL commentedAug 11, 2019
edited
Loading

@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 introducingResolveClassPass 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
Copy link
MemberAuthor

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
Copy link
Contributor

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

@ro0NL
Copy link
Contributor

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

nicolas-grekas added a commit that referenced this pull requestAug 20, 2019
…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-grekasnicolas-grekas merged commited590ca intosymfony:4.3Aug 20, 2019
@nicolas-grekasnicolas-grekas deleted the di-revert branchAugust 23, 2019 11:45
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

4 participants

@nicolas-grekas@fabpot@ro0NL@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp