Skip to content

[DependencyInjection] Show error when a null value is provided in the exclude list #46835

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
nicolas-grekas merged 1 commit into from
Aug 3, 2022
Merged

[DependencyInjection] Show error when a null value is provided in the exclude list #46835

nicolas-grekas merged 1 commit into from
Aug 3, 2022

Conversation

@alexislefebvre
Copy link
Contributor

@alexislefebvre alexislefebvre commented Jul 3, 2022 •
edited by nicolas-grekas
Loading

Q A
Branch? 6.2
Bug fix? no
New feature? no
Deprecations? no
Tickets Test for #46833
License MIT
Doc PR

Avoid an exception when null values are provided in the $excludePatterns array.

@carsonbot carsonbot added this to the 4.4 milestone Jul 3, 2022
@alexislefebvre alexislefebvre force-pushed the add-test-with-wrong-include-46833 branch from 486cc99 to ff45787 Compare July 21, 2022 11:05
@alexislefebvre alexislefebvre changed the title add test with wrong include, see #46833 Avoid an exception when null values are provided in the $excludePatterns array Jul 21, 2022
@alexislefebvre alexislefebvre mentioned this pull request Jul 21, 2022
Closed
@alexislefebvre alexislefebvre force-pushed the add-test-with-wrong-include-46833 branch 2 times, most recently from 3d8b562 to 160eb73 Compare July 21, 2022 11:14
@stof
Copy link
Member

stof commented Jul 21, 2022

I would rather fail with a good error message explaining you your mistake rather than silently ignoring the invalid pattern.

@carsonbot carsonbot changed the title Avoid an exception when null values are provided in the $excludePatterns array [DependencyInjection] Avoid an exception when null values are provided in the $excludePatterns array Jul 21, 2022
@alexislefebvre alexislefebvre force-pushed the add-test-with-wrong-include-46833 branch from 160eb73 to 63cfe5c Compare July 21, 2022 11:26
@alexislefebvre
Copy link
Contributor Author

alexislefebvre commented Jul 21, 2022

@stof Right, I force-pushed a check that will throw an error instead of ignoring it.

If it's ok for you, tests will have to be updated to handle this case in a new test.

@alexislefebvre alexislefebvre force-pushed the add-test-with-wrong-include-46833 branch from 63cfe5c to 0b2f7ad Compare July 21, 2022 11:30
@alexislefebvre alexislefebvre changed the title [DependencyInjection] Avoid an exception when null values are provided in the $excludePatterns array [DependencyInjection] Show error when a null value is provided in the exclude list Jul 21, 2022
fabpot
fabpot requested changes Jul 21, 2022
View reviewed changes
@fabpot
Copy link
Member

fabpot commented Jul 21, 2022

I think this should be rebased on 6.2.
Can you update the tests?

@alexislefebvre
Copy link
Contributor Author

alexislefebvre commented Jul 21, 2022

What if using an empty item in another part of the config throw another error in another part of Symfony? Should the config validator be more strict? As I wrote in #46833, the YAML (or XML, etc.) is still valid.

@stof
Copy link
Member

stof commented Jul 21, 2022

the Yaml being valid for the Yaml parser does not mean that the content of this Yaml is a valid input for the DependencyInjection file loader. That's why it is the FileLoader doing its validation on the input it expects.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 27, 2022

Tests fail, see failures + fabbot report also.

It'd be better to create dedicated test cases to me. Can you please update the PR to do so?

@nicolas-grekas nicolas-grekas modified the milestones: 4.4, 6.2 Aug 2, 2022
@alexislefebvre alexislefebvre force-pushed the add-test-with-wrong-include-46833 branch from 9182174 to fc6a0d0 Compare August 2, 2022 18:39
@alexislefebvre alexislefebvre changed the base branch from 4.4 to 6.2 August 2, 2022 18:39
@alexislefebvre alexislefebvre force-pushed the add-test-with-wrong-include-46833 branch 2 times, most recently from dec430e to 6bd7f57 Compare August 2, 2022 18:45
@alexislefebvre
Copy link
Contributor Author

alexislefebvre commented Aug 2, 2022

The branch has been rebased and tests have been added.

@nicolas-grekas nicolas-grekas force-pushed the add-test-with-wrong-include-46833 branch from d62918f to e034be0 Compare August 3, 2022 12:06
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 3, 2022

Thank you @alexislefebvre.

@nicolas-grekas nicolas-grekas merged commit 667e635 into symfony:6.2 Aug 3, 2022
@alexislefebvre alexislefebvre deleted the add-test-with-wrong-include-46833 branch November 1, 2022 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot requested changes

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

@lyrixx lyrixx Awaiting requested review from lyrixx

@yceruto yceruto Awaiting requested review from yceruto

@wouterj wouterj Awaiting requested review from wouterj

@chalasr chalasr Awaiting requested review from chalasr

@dunglas dunglas Awaiting requested review from dunglas

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees

No one assigned

Labels

Bug DependencyInjection Status: Reviewed

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

6 participants

@alexislefebvre @stof @fabpot @nicolas-grekas @xabbuh @carsonbot