Skip to content
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

Add dateTimeModify return type provider #8462

Merged
merged 2 commits into from Sep 14, 2022
Merged

Add dateTimeModify return type provider #8462

merged 2 commits into from Sep 14, 2022

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet force-pushed the dateTimeModify branch 2 times, most recently from 575ec21 to f58545d Compare September 6, 2022 09:15
@VincentLanglet
Copy link
Contributor Author

I think Ci failure are not related

@VincentLanglet VincentLanglet mentioned this pull request Sep 6, 2022
Closed
@AndrolGenhald
Copy link
Collaborator

I think this is fine, as discussed in #8454 the false return should only depend on the $modifier argument (as well as DATE_TIMEZONEDB and php_date_parse_tzfile_wrapper, but it's probably safe to assume those won't cause errors unless there's a major issue with the PHP installation).

It's also possible to get a false return in 7.x (error in 8.x) if the constructor doesn't initialize the class correctly, but as far as I'm aware the only way to do this is with reflection, and if you're using newInstanceWithoutConstructor() you're seriously playing with fire and I don't think we should care.

@AndrolGenhald
Copy link
Collaborator

@orklah any idea what's up with the Shepherd issue? I thought I fixed it earlier so that we can't accidentally use IssueBuffer::add directly anymore, so I'm not sure how it's turning up vendor code.

@orklah
Copy link
Collaborator

orklah commented Sep 7, 2022

Most probably Psalm is retrieving the latest version of PHP-Parser that came out a few days ago and it broke something. We'll need to take a look at it

Otherwise I'm fine with the PR until we can have a working example of what we need to guard against

@VincentLanglet
Copy link
Contributor Author

Most probably Psalm is retrieving the latest version of PHP-Parser that came out a few days ago and it broke something. We'll need to take a look at it

Otherwise I'm fine with the PR until we can have a working example of what we need to guard against

Tests are fixed @orklah :)

@orklah orklah added the release:feature The PR will be included in 'Features' section of the release notes label Sep 14, 2022
@orklah orklah merged commit 9ed9c4b into vimeo:4.x Sep 14, 2022
@orklah
Copy link
Collaborator

orklah commented Sep 14, 2022

Thanks!

DanielBadura added a commit to patchlevel/event-sourcing that referenced this pull request Sep 23, 2022
@DanielBadura
Update phpstan baseline to remove an entry. Also added one entry to p…
08c19ea 
…salm baseline regarding DateTimeImmutable::modify which can result `false` in some cases. In our case this is not possible and should also be fixed with the next release.

Ref: https://psalm.dev/r/8f44d5ca2d
Also ref: vimeo/psalm#8462
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrolGenhald AndrolGenhald AndrolGenhald approved these changes

@orklah orklah orklah left review comments

Assignees
No one assigned
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

\DatetimeImmutable::modify signature change (vs \Datetime::modify)
3 participants
@VincentLanglet @AndrolGenhald @orklah