Declare strict_types=1 in every file #763

Art4 · 2022-11-04T10:46:32Z

By default, PHP will coerce values of the wrong type into the expected scalar type declaration if possible. It is possible to enable strict mode, so only a value corresponding exactly to the type declaration will be accepted, otherwise a TypeError will be thrown.

This PR enables strict mode in all files. In most of the classes, everything works as expected but in a few classes I had to cast some values explicitly. I could even find and fix some bugs:

Fixed a bug in the Misc class
Fixed SimplePie::get_image_height() and get_image_width() returning int|null instead of int|float|null, introduced in 33a8f76

I've also checked and fixed most of the errors for PHPStan Level 1.

I will keep this PR as draft until the other open PRs are merged so I can later add the strict types in the new classes and interfaces created by theses PRs.

src/SimplePie.php

jtojnar · 2022-11-13T13:10:28Z

src/IRI.php

@@ -373,7 +375,7 @@ protected function remove_dot_segments($input)
            // C: if the input buffer begins with a prefix of "/../" or "/..", where ".." is a complete path segment, then replace that prefix with "/" in the input buffer and remove the last segment and its preceding "/" (if any) from the output buffer; otherwise,
            elseif (strpos($input, '/../') === 0) {
                $input = substr($input, 3);
-                $output = substr_replace($output, '', strrpos($output, '/'));
+                $output = substr_replace($output, '', intval(strrpos($output, '/')));


It might be clearer to use something more explicit like ($pos = strrpos($output, '/')) === false ? 0 : $pos or at least strrpos($output, '/') ?: 0.

But since this is going away in the future it might be fine as is.

My motivation to improve this soon to be deprecated class is not very high. 😅

src/IRI.php

jtojnar · 2022-11-13T13:16:50Z

src/IRI.php

@@ -817,7 +819,7 @@ public function set_authority($authority, $clear_cache = false)
        } else {
            $iuserinfo = null;
        }
-        if (($port_start = strpos($remaining, ':', strpos($remaining, ']'))) !== false) {
+        if (($port_start = strpos($remaining, ':', intval(strpos($remaining, ']')))) !== false) {


Same here – with casts, developer needs to consider both possible initial return values and the mapping performed by the cast. Explicit conversion would make it more transparent.

May I ask for an example?

src/Parse/Date.php

src/HTTP/Parser.php

src/IRI.php

Returning float instead of int was wrongly introduced in commt simplepie@33a8f76 According to the RSS 2.0 spec height and width must be the number of pixels. see https://www.rssboard.org/rss-specification#ltimagegtSubelementOfLtchannelgt

src/IRI.php

src/SimplePie.php

@jtojnar

* bump version to 1.8.0 * Update CHANGELOG.md * Fix version tags in deprecated messages * fix version in old deprecation messages * Fix typo see comment from @jtojnar in #752 * Add comment for DataCache interface see comment from @jtojnar in #752 * Update CHANGELOG.md for #760, #764 and #765 * Update CHANGELOG.md for #762, #767 and #763 * Update CHANGELOG.md for #768 and #770 * Update release date * Update CHANGELOG.md for #769 and #771 * Update CHANGELOG.md for #766

jtojnar · 2023-06-09T11:31:42Z

@mblaney Could you please create one-dot-eight branch from 1.8.0 so that we could revert this PR there for 1.8.1 to fix #810?

Art4 · 2023-06-09T18:07:47Z

Why should we revert the complete PR? A better option would be to remove declare(strict_types=1); from a specific file, instead from all files.

jtojnar · 2023-06-09T18:21:17Z

Because it is impossible to tell which files are actually broken without a complete test coverage or static analysis. And since most of our functions currently only have argument types specified using a PHPDoc, incorrectly typed value can cause an error arbitrarily deep, when they are passed to native PHP functions.

So far we have seen four or five different instances of this, IIRC, and I doubt those are all of the issues.

Art4 · 2023-06-09T18:33:00Z

Because it is impossible to tell which files are actually broken without a complete test coverage or static analysis. And since most of our functions currently only have argument types specified using a PHPDoc, incorrectly typed value can cause an error arbitrarily deep, when they are passed to native PHP functions.

So far we have seen four or five different instances of this, IIRC, and I doubt those are all of the issues.

I would like to see bug fixes releases more often where the issues are fixed instead of removing the declare statement.. But I'm afraid that the chances are not very high that this will happen. Maybe we could speed up the release for 1.9.0 instead?

jtojnar · 2023-06-09T19:03:42Z

I would like to see bug fixes releases more often where the issues are fixed instead of removing the declare statement.. But I'm afraid that the chances are not very high that this will happen.

Same here.

Maybe we could speed up the release for 1.9.0 instead?

Well, I see the following options:

Remove strict_types everywhere until we have high certainty that the issues do not occur.
- This is the easiest solution and could be even done in 1.8 branch.
Remove strict_types just from files where the issue has been reported.
- Potentially will need to be repeated as new reports come.
Look for all files where the problematic calls are and remove strict_types.
- How? Maybe functions passing values of untyped arguments to native PHP functions? Sounds like a lot of work. Probably best left for static analysis.
Add tests for all possible argument types of all functions.
- Even 100 % line coverage is not guarantee of lack of errors. But might still give us some degree of assurance.
- Still lot of work.
Raise PHPStan level (IIRC, 8 or 9 should catch these kind of issues)
- This will also take time but I think this is best option for 1.9.0.

Art4 added 4 commits November 4, 2022 11:06

declare strict_types=1 in every file

d568d8c

declare strict_types=1 in IRI and Parse Date class

83823a4

declare strict_types=1 in Misc class, fix bug

05ae6c7

declare strict_types=1 in SimplePie class

2217f70

pull-request-size bot added the size/L label Nov 4, 2022

Art4 mentioned this pull request Nov 4, 2022

Possible roadmap #731

Open

48 tasks

Art4 added 3 commits November 4, 2022 12:39

BC for PHP < 8.0

2b8092e

Strip declare statement in compiled file

f111812

Fix many errors for PHPStan Level 1

968e9b4

Art4 marked this pull request as ready for review November 7, 2022 09:27

Art4 marked this pull request as draft November 7, 2022 09:50

jtojnar reviewed Nov 13, 2022

View reviewed changes

src/SimplePie.php Outdated Show resolved Hide resolved

jtojnar reviewed Nov 13, 2022

View reviewed changes

Art4 added 5 commits November 14, 2022 14:54

do not round() int

e3938f0

Update CHANGELOG.md

a503ec6

clarify some statements

8282b21

Fix $encoded declatation

d323783

jtojnar reviewed Nov 14, 2022

View reviewed changes

src/IRI.php Outdated Show resolved Hide resolved

jtojnar reviewed Nov 14, 2022

View reviewed changes

src/SimplePie.php Show resolved Hide resolved

Art4 added 3 commits November 14, 2022 16:06

Fix $valid declaration

635d018

Merge branch 'master' into declare-strict-types

c5ee2a7

fix CS by running php-cs-fixer

867def2

Art4 marked this pull request as ready for review December 5, 2022 08:05

jtojnar approved these changes Dec 5, 2022

View reviewed changes

mblaney merged commit efe70f4 into simplepie:master Dec 13, 2022

Art4 deleted the declare-strict-types branch December 13, 2022 10:18

Art4 added a commit to Art4/simplepie that referenced this pull request Dec 13, 2022

Update CHANGELOG.md for simplepie#762, simplepie#767 and simplepie#763

5064aaa

jtojnar added a commit to jtojnar/simplepie that referenced this pull request Jan 12, 2023

fixup! Declare strict_types=1 in every file (simplepie#763)

3f39576

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Declare strict_types=1 in every file #763

Declare strict_types=1 in every file #763

Art4 commented Nov 4, 2022 •

edited

jtojnar Nov 13, 2022

Art4 Nov 14, 2022

jtojnar Nov 13, 2022

Art4 Nov 14, 2022

jtojnar commented Jun 9, 2023

Art4 commented Jun 9, 2023

jtojnar commented Jun 9, 2023

Art4 commented Jun 9, 2023

jtojnar commented Jun 9, 2023

Declare strict_types=1 in every file #763

Declare strict_types=1 in every file #763

Conversation

Art4 commented Nov 4, 2022 • edited

jtojnar Nov 13, 2022

Choose a reason for hiding this comment

Art4 Nov 14, 2022

Choose a reason for hiding this comment

jtojnar Nov 13, 2022

Choose a reason for hiding this comment

Art4 Nov 14, 2022

Choose a reason for hiding this comment

jtojnar commented Jun 9, 2023

Art4 commented Jun 9, 2023

jtojnar commented Jun 9, 2023

Art4 commented Jun 9, 2023

jtojnar commented Jun 9, 2023

Art4 commented Nov 4, 2022 •

edited