Declare strict_types=1 in every file by Art4 · Pull Request #763 · simplepie/simplepie

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

            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

            $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 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

Art4 added this to the 1.9.0 milestone Sep 30, 2024

Art4 mentioned this pull request Jun 29, 2025

Changelog: Mention some more changes since 1.8.0 #923

Closed

Art4 removed this from the 1.9.0 milestone Jul 7, 2025

Conversation

Art4 commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jtojnar Nov 13, 2022

Choose a reason for hiding this comment

Uh oh!

Art4 Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtojnar Nov 13, 2022

Choose a reason for hiding this comment

Uh oh!

Art4 Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtojnar commented Jun 9, 2023

Uh oh!

Art4 commented Jun 9, 2023

Uh oh!

jtojnar commented Jun 9, 2023

Uh oh!

Art4 commented Jun 9, 2023

Uh oh!

jtojnar commented Jun 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Art4 commented Nov 4, 2022 •

edited

Loading