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.

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

Fix some 8.2 compatibility issues in Date validator #149

Merged
merged 6 commits into from Sep 13, 2022

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Sep 12, 2022

Q A
Bugfix yes
QA yes

Description

Testing a local project under 8.2-RC reveals a problem specific to the Date validator, where DateTime::getLastErrors() is potentially false. Even given https://3v4l.org/HBq0q - I can't get a test to actually return false from this method because it's 8.2 specific, so verifying the fix is a tough one.

Looking at https://3v4l.org/3QsKY - Only 8.2 returns false after a date operation that causes no errors.

A couple of other tests had complaints about dynamic properties and I've silenced a number of tests that don't perform any assertions depending on the data providers.

Add anonymous classes in tests to avoid setting dynamic properties.
Also, inform PHPUnit that some tests will not perform assertions making the output of PHPUnit less verbose.

Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
@gsteel gsteel added this to the 2.24.0 milestone Sep 12, 2022
@gsteel gsteel changed the title 8.2 fwd compat Fix some 8.2 compatibility issues Sep 12, 2022
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

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

Does psalm already support that false return value of DateTime#getLastErrors?

If so, its absolutely okay to early return without actually having a test which verifies if the methods return value is false is due to the fact that DateTime::getLastErrors returns an array with warning_count/error_count being positive-int or due to the return value false.

Overall, LGTM.

psalm-baseline.xml Show resolved Hide resolved
test/File/ExcludeExtensionTest.php Outdated Show resolved Hide resolved
test/File/ExistsTest.php Outdated Show resolved Hide resolved
test/File/ExtensionTest.php Outdated Show resolved Hide resolved
test/File/HashTest.php Outdated Show resolved Hide resolved
test/File/NotExistsTest.php Outdated Show resolved Hide resolved
test/File/Sha1Test.php Outdated Show resolved Hide resolved
test/File/SizeTest.php Outdated Show resolved Hide resolved
test/File/WordCountTest.php Outdated Show resolved Hide resolved
test/File/Crc32Test.php Outdated Show resolved Hide resolved
@gsteel
Copy link
Member Author

gsteel commented Sep 12, 2022

Related: vimeo/psalm#8478

Signed-off-by: George Steel <george@net-glue.co.uk>
@gsteel gsteel requested a review from boesing September 12, 2022 14:35
@boesing
Copy link
Member

boesing commented Sep 12, 2022

@gsteel you actually have to disable ignoreInternalFunctionFalseReturn in psalm config.

Sadly, thats enabled by default.

Ref: vimeo/psalm#7758 (comment)

@gsteel
Copy link
Member Author

gsteel commented Sep 12, 2022

@boesing - this should be good to go. The conditional in Date validator is required for PHP 8.2 though due to real errors such as array offset on bool… so I guess we'll just leave it in the baseline.

I did take a quick look at disabling ignoreInternalFunctionFalseReturn in psalm.xml but that yielded another 200 errors. Happy to disable it and expand the baseline if you'd like to see that here?

Is it worth trawling through the options and coming up with a standard config for all of the components do you think?

@Ocramius
Copy link
Member

Is it worth trawling through the options and coming up with a standard config for all of the components do you think?

Yes, but not here :D

@Ocramius Ocramius self-assigned this Sep 13, 2022
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @gsteel!

@Ocramius Ocramius changed the title Fix some 8.2 compatibility issues Fix some 8.2 compatibility issues in Date validator Sep 13, 2022
@Ocramius Ocramius merged commit b0e8bc7 into laminas:2.24.x Sep 13, 2022
@gsteel gsteel deleted the 8.2-fwd-compat branch September 13, 2022 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants