Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[DependencyInjection] More bullet-proof expression evaluation#59976
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
base:6.4
Are you sure you want to change the base?
[DependencyInjection] More bullet-proof expression evaluation#59976
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
4266c4a
to1a7bf06
Comparecarsonbot commentedMar 14, 2025
Hey! Thanks for your PR. You are targeting branch "7.2" but it seems your PR description refers to branch "6.4, 7.2". Cheers! Carsonbot |
f22ebf9
to0dcb2e6
CompareIn some scenarios evaluating expression can lead to TypeError (unresolved DI params being strings, not expected integers etc).
0dcb2e6
to247f0f9
Compare...nent/DependencyInjection/Tests/Fixtures/CheckTypeDeclarationsPass/ExpectsIntegerArgument.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
`composer.json`'s requirement for PHP is `PHP >= 8.1`, so even though GH Actions does not contain PHP 8.1 in the matrix (which is wrong IMHO), let's stick with `readonly` on property level.
I've additionally fixed PHP 8.1 compatibility ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So, the error you describe on Twitter happens before env vars are unresolved, while running the expression.
I feel like the solution that's proposed at the moment is way to much. What if the error is a parse error or anything else that'd better be caught at linting time?
Sorry I don't have a proposal, that's just the question that comes up at the moment :)
@@ -0,0 +1,20 @@ | |||
<?php | |||
declare(strict_types=1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
to be replaced by the licence header (same below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
also in fixture file?
if so, we could run the Fixer to apply this for other fixture files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the thing is we'd like the licence header in fixtures (because why not), but then, no need to enforce other rules I'd say (at least we'll get many false-positives if we do)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and PHP-CS-Fixer only allows to exclude files entirely, not to override the config being applied to some files to apply only some of the rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, we definitely need to introduce skipping rules like ECS has, I like it a lot.
But you can also have a separate ruleset for fixtures and run Fixer twice, pointing to the config file.
@nicolas-grekas I've been thinking about your question last 2 days and my initial opinion did not change -
In our case it's runtime issue, but for other scenarios it can be actually language-level issue - none of them should break the DI linting (maybe flag for this would be a good idea, so you could run |
nicolas-grekas commentedMar 20, 2025 • 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.
What about checking the error message and ignoring only the ones we know we want to ignore? |
Up to follow my last suggestion@Wirone? |
Uh oh!
There was an error while loading.Please reload this page.
As askedhere, we faced weird issue recently. I was able to reproduce it with minimal DI setup in
CheckTypeDeclarationsPassTest
.