10000 [HttpKernel] allow narrow type of not nullable getResponse when hasResponse has been called by shyim · Pull Request #58241 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel] allow narrow type of not nullable getResponse when hasResponse has been called #58241

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
Oct 9, 2024
Merged

[HttpKernel] allow narrow type of not nullable getResponse when hasResponse has been called #58241

merged 1 commit into from
Oct 9, 2024

Conversation

shyim
Copy link
Contributor
@shyim shyim commented Sep 12, 2024 •
edited by nicolas-grekas
Loading
Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT 
if ($event->hasResponse()) {
    return $event->getResponse();
}

PHPStan understands now inside the if that Response is not nullable anymore

@carsonbot carsonbot added this to the 5.4 milestone Sep 12, 2024
@shyim shyim changed the title allow narrow type of not nullable getResponse when hasResponse has be… [HttpKernel] allow narrow type of not nullable getResponse when hasResponse has be… Sep 12, 2024
xabbuh
xabbuh approved these changes Sep 12, 2024
View reviewed changes
Copy link
Member
@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

but for 7.2 as this is not a bugfix

@carsonbot carsonbot changed the title [HttpKernel] allow narrow type of not nullable getResponse when hasResponse has be… allow narrow type of not nullable getResponse when hasResponse has be… Sep 12, 2024
@xabbuh xabbuh modified the milestones: 5.4, 7.2 Sep 12, 2024
derrabus
derrabus approved these changes Sep 12, 2024
View reviewed changes
Copy link
Member
@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

for 7.2

@wouterj
Copy link
Member
wouterj commented Sep 13, 2024 •
edited
Loading

As far as I know, we do not accept static analysis annotations that are not generally supported across Psalm, PHPStan and PhpStorm (see also the list of accepted annotations in the Symfony standards).

It seems like assert-if-true is not supported by, at least, Psalm. So I'm 👎 , although the added annotation is correct (it might be worth submitting this as a stub to the PHPStan Symfony plugin).

@stof
Copy link
Member
stof commented Sep 13, 2024

Psalm does support the assert-if-true annotation. However, by default, Psalm does not memoize the type for getters, which is why it has no effect in your code snippet. See https://psalm.dev/docs/running_psalm/configuration/#memoizemethodcallresults for the configuration and https://psalm.dev/r/561c4c3a87 for the same code snippet with this setting turned on.

phpstan has a different default than psalm regarding this option.

@wouterj
Copy link
Member
wouterj commented Sep 13, 2024 •
edited
Loading

Ah, of course. Thanks for correcting me, @stof.

Let's do it then 👍 , but with the @psalm- prefix to be consistent with the other annotations in the Symfony code base (although the prefix doesn't change anything in behavior).

@stof
Copy link
Member
stof commented Sep 13, 2024 •
edited
Loading

The only change in behavior is for the case where you write something wrong in the tag: both phpstan and psalm are reporting errors when writing an invalid tag with their own prefix while they silently discard an invalid tag using the prefix of the other tool (because maybe this tag is actually valid in the other tool and they don't have feature parity).
The recommended setup is to write annotation using the prefix of the tool that we use ourselves (and we are partially using psalm in our CI, which is why we use psalm prefixes)

@stof
Copy link
Member
stof commented Sep 13, 2024

For reference, see https://psalm.dev/docs/annotating_code/adding_assertions/#asserting-return-values-of-methods for the Psalm documentation about the exact conditions for @psalm-assert-if-true ... $this->someGetter() to work. There are cases that work without memoizeMethodCallResults but this class does not correspond to those cases.

@nicolas-grekas nicolas-grekas changed the title allow narrow type of not nullable getResponse when hasResponse has be… allow narrow type of not nullable getResponse when hasResponse has been called Oct 9, 2024
@carsonbot carsonbot changed the title allow narrow type of not nullable getResponse when hasResponse has been called [HttpKernel] allow narrow type of not nullable getResponse when hasResponse has been called Oct 9, 2024
@nicolas-grekas nicolas-grekas changed the base branch from 5.4 to 7.2 October 9, 2024 15:02
@nicolas-grekas
Copy link
Member

Thank you @shyim.

@nicolas-grekas nicolas-grekas merged commit 52f1436 into symfony:7.2 Oct 9, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof requested changes

@OskarStark OskarStark OskarStark approved these changes

@derrabus derrabus derrabus approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Bug HttpKernel
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.
8 participants
@shyim @wouterj @stof @nicolas-grekas @OskarStark @derrabus @xabbuh @carsonbot
0