My first post, php8 and phpstan

General Discussion
1

Hello all,

First of all,
sorry I post here, because I confuse to select the forum category, wether to put here or into petition.

I just found ClassicPress couple days ago, while doing some polyglot translation at wordpress.org. I already use wp since 2009 and modify some theme and plugins for personal use and my clients. Most of theme heavily modified because I need to meet the SEO requirements.

I already install CP at my localhost. It’s smooth and OK, no error found. I did some commenting and creating new post, for those simple functions, CP is OK.
My box configs are Debian 11 Bullseye, PHP8.0-FPM, nginx, Mariadb 10.6, Galera 4.

I already forking the github repo and doing some checking using PHPStan. For the time being, I already find some suggestions. FYI, I already tried couple of tools, but only PHPStan and PHPUnit that easy enough and can support my projects.

Please see my fork repo at https://github.com/steamboatid/ClassicPress.
I already check all of files in the root directory, and couple of file in the wp-includes.

Please give me your opinion about static analysis using PHPStan. If you agree, I will continue the checking, including fixing the code starting from wp-includes, then continue to wp-admin, and wp-content.
For the time being, I only use level 3, of course with some exclusions. (Please see the phpstan.neon file)
If this succeded, we can continue to higher level later, but I think level 5 is more than enough. In fact, all of my project only valid at level 3.

Later,
maybe we can continue our effort to check all of the plugin and theme. Especially the plugin and theme that listed in the CP website.

That’s all for now, please give your opinion and thought about PHPStan.
I’m just an old guy who try to catch the bullet train. LOL.

Regards,
dwi

5 Likes
2

I think @james is the right person to tag here.
As concerns if this should be a petition, I don’t know. I’m positive that help in making CP compatible with PHP 8 is needed, and even if PHP 8 will be implemented far away down the line having someone starting to check the code and prepare for that is useful because one day or another the job has to be done.
I am not a dev, but lurking around (mostly during the weekly core meetings) I have seen mentions of PHPUnit tests, so I guess that is the tool our core team is using, but again here James can give a more detailed answer.
Thanks for volunteering on this task!

2 Likes
3

Welcome to the community :slightly_smiling_face:

PHP 8 support is slated for the upcoming version 1.4.0.

This is the main issue tracking PHP 8 support, and there are several other issues tagged PHP 8.

Testing everything is definitely welcome. I think 1.4.0-RC1 is not that far off.

James is the core lead, so he’ll be able to provide more information. Core Slack channel is also a great way to get questions answered.

5 Likes
4

Hi @viktor @ElisabettaCarrara,
thanks for the welcome message.

@viktor
Yes, I already check the tag at github.
According to the reported issues, actually, it’s really easy to find that kind of bug using PHPStan.

So, scanning thorughly all code using PHPStan will ensure that we wont facing the same bug in the future. :grinning:

2 Likes
5

The problem with using a tool like PHPStan on ClassicPress (or WordPress for that matter) is that there are a lot of false positives (things detected that aren’t really an issue). Adding the global annotations to files like wp-activate.php is one example of this. This file is always loaded in a global context, but PHPStan doesn’t have any way to know that. So some of these changes aren’t really necessary and just put CP further out of sync from WP, making it harder to backport other changes in the future.

The CP/WP codebase is far from perfect and that also leads these tools to suggest changes that would break the logic of the code. It looks like this and this may be examples of such breaking changes.

This kind of analysis is still a good idea though, it’s just something to be very careful with. I think a better strategy here would be to follow the work WP is doing with this tool and adopt the same specific code changes that they make. We should also take advantage of the much larger amount of testing that WP is able to do compared to our community. Here is a Trac ticket to follow along with (#52217 (Fix code issues identified by PHPStan) – WordPress Trac), which already has a few commits that have been made, and here are the ClassicPress instructions for backporting WP changes: ClassicPress/.github/CONTRIBUTING.md at e2d7ea07e2d0ebac4c6602b85e867056d24656ad · ClassicPress/ClassicPress · GitHub

Finally as far as PHP 8 compatibility a more immediately useful approach is to get all of the PHPUnit tests updated and working under PHP 8. This work is in progress and we could use your help reviewing this huge PR: https://github.com/ClassicPress/ClassicPress/pull/815

1 Like
6

@james
about the first code
$taxonomy is object while $_taxonomy is array, so I believe accessing property from $_taxonomy is wrong, is’n it?

the second code
please take alook at the function description, $group is defined as optional int. the int is not used yet in functions parameter just because backward compatibility especially with PHP 5x.
Other than that, please also take alook at how $group variable used in other functions.
I believe its wrong, if we give $group default value as false. CMIIW

For the global variable declaration,
it is safer, if the caller file declare it first, not the library. We will not register all global variables of the library in the caller file, we simply register the mentioned one.
In that way, it’s easier to read the current file, even though we dont read all of the library files.

About false positive,
that’s why I only using level 3. Maximum level at PHPStan is 8. It’s good if we can reach more than level 3, if not, It’s OK. Level 3 is GOOD enough. It help me much from undefined variables, typos, unbalanced if blocks, etc.

FYI,
I just checking class-wp-query.php, take a look at variable $climits at method get_posts.
I think that huge mistake, when $climits is empty. Can you imagine if $climits or maybe $limits is empty?
Those kind of bug maybe never get enough attention from core dev, just because no one ever report that.
That kind of code can not be found by human eyes, we must use tools to do it.

When we can see a bug via user report or phpunit, it’s just because user use the software. It’s blackbox inspection. So, why not we do whitebox inspection ? :grinning:

About the backporting, let me take a look later.
I just check my box, PHPUnit is already installed, but it’s version 9.5. I will downgrade it first to 8 or 7 as discussed in the github.

7

The default for this variable is false, and 0 is another special value which has a different meaning. They cannot be interchanged. See: cp/src/wp-includes/class.wp-scripts.php at 54ba00072cee898fccc85ca39756dce0e2304818 · steamboatid/cp · GitHub

So the thing to fix here would be the type annotation for the parameter, but this is only obvious when you consider the rest of the code for this specific change.

These files like wp-activate.php are not ever called from libraries. There may be a small readability benefit but it is not worth the risk of breakage and the divergence from WP.

For an example of where making this change would break something: in wp-includes/version.php where the $wp_version and other variables are set, this sometimes needs to happen in the global context, and other times (during the core update process) this file is included and the variables need to be declared in local scope. Static analysis tools can’t know about things like that, and PHPStan will give the wrong recommendation here.

Being aware of these gotchas requires an expert level of knowledge of the WP/CP core code. I only know a few of them but I know there are many more.

The complexity involved here also means that each of these changes needs to be evaluated separately, and we just don’t have the resources to do that. I suggest that you work with the WP efforts here first, and/or help us backport any changes that come out of that work.

8

I did that.
in that case, the deafult value in the code is wrong.

it’s happening on almost all of wp-includes/class*php, please check my commit at this and this

9

I’m doing now, although haven’t downgrading my phpunit yet, LOL :grinning:
I’m just involves with small diffs, and it can be done manually without any tools, simply read the code and pasting with small modifs according to standard and existing codebase.

also, about the code standard,
I’m seeing couple of files that not comply with the standard.
How that can be happened at WP and CP?

1 Like
10

No, the type annotation is wrong. It should be int|false or int|bool. false is a valid value for this parameter and it means something very different than 0.

WP did a huge reformatting commit a while ago: https://core.trac.wordpress.org/changeset/42343

We should be looking to do the same thing, but again we don’t have the resources to do this all at once. I think the following is an approach that will work better for us:

  • Open a PR to fix all the issues for one PHPCS rule at a time (for example, indentation).
  • Measure the number of lines that are now the same as the WP code base, each time we make formatting fixes this should go up. This tells us that we are getting our code more into sync with WP’s formatting.
  • Enable the GitHub Actions checks for that specific rule so that PRs which introduce invalid formatting under that rule will fail.
  • Repeat the above steps for other formatting rules.

We would need someone to lead these efforts, and start by opening a “master issue” on GitHub with a list of all the formatting rules we want to enforce.

1 Like