8000 [DependencyInjection] Better exception when a configurator is not type hinted by lyrixx · Pull Request #40930 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] Better exception when a configurator is not type hinted #40930

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
Merged
merged 1 commit into from
May 9, 2021
Merged

[DependencyInjection] Better exception when a configurator is not type hinted #40930

merged 1 commit into from
May 9, 2021

Conversation

lyrixx
Copy link
Member
@lyrixx lyrixx commented Apr 23, 2021
Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

This would help with such BC break:
markitosgv/JWTRefreshTokenBundle#241

@lyrixx lyrixx requested a review from Nyholm April 23, 2021 15:11
Nyholm
Nyholm approved these changes Apr 23, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are a star. Thank you!

@stof
Copy link
Member
stof commented Apr 23, 2021

shouldn't we assume it is a ContainerConfigurator and trigger a deprecation warning instead ? Otherwise, it is actually a BC break.

@Nyholm
Copy link
Member
Nyholm commented Apr 23, 2021

I think you are correct @stof.. But that is boarder line overkill. =)

@stof
Copy link
Member
stof commented Apr 23, 2021

@Nyholm ContainerConfigurator already exists since Symfony 4.4 (maybe even earlier 4.x but I haven't checked). So I assume that this bundle might not be the only one out there using the feature without a typehint on the callable.

@nicolas-grekas
Copy link
Member

I think you are correct @stof.. But that is boarder line overkill. =)

I agree that this would be overkill. Since this failure happens at compile time, we're not going to expose prod servers, so we can do it IMHO.

@nicolas-grekas nicolas-grekas added this to the 5.3 milestone May 1, 2021
@fabpot
Copy link
Member
fabpot commented May 9, 2021

Thank you @lyrixx.

@fabpot fabpot merged commit 3c76625 into symfony:5.x May 9, 2021
@lyrixx lyrixx deleted the dic-exception branch May 9, 2021 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@Nyholm Nyholm Nyholm approved these changes

Assignees
No one assigned
Labels
DependencyInjection Ready Status: Reviewed
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.
6 participants
@lyrixx @stof @Nyholm @nicolas-grekas @fabpot @carsonbot
0