Consider bumping pre-commit checker on plugins to PHP 7.X

[Ipstenu]

Currently we have pre-commit checks set for PHP 7.0, which is great ... except people are starting to use PHP 7.1 and up for their code and getting burned by not being able to commit code to SVN becuase of that.

7.0 is only getting security reports through end of 2018, so it's probably time to have a gander at the possibility of bumping to 7.1 (or if we want to be ahead of the game, 7.2...)

Things to consider:

1) Will bumping the checker have an ADVERSE impact on plugins written for 5.2.4?

2) Do we have a way to see what PHP versions plugins use?

3) If 2 is yes, do more plugins use 5.2.4 or 7.1+?

[tellyworth]

More WordPress sites use PHP 7.2 than PHP 5.2. I'll see if I can dig up stats on plugin compat.

It seems like the best way to proceed here is to bump the requirement and listen to feedback.

Can anyone suggest a compelling reason not to?

[dd32]

1) Will bumping the checker have an ADVERSE impact on plugins written for 5.2.4?

Shouldn't do. When we bumped it to 7.0 that would have added some (but that was okay)

Looking at the PHP 7.1/7.2 backward incompatible changes, I see nothing that should change linting of older code

• ​http://au2.php.net/manual/en/migration71.incompatible.php
• ​http://au2.php.net/manual/en/migration72.incompatible.php

2) Do we have a way to see what PHP versions plugins use?

We can run manual lookups for that kind of thing, there are roughly 40 plugins whose requires_php is PHP 7.1 or 7.2.

3) If 2 is yes, do more plugins use 5.2.4 or 7.1+?

Literally thousands more plugins are marked as requiring PHP 5.0~5.2.4. I don't think that has any impact here.

I see no reason why the linting for plugin commits shouldn't be increased to PHP 7.2 (which is what we're currently running for web nodes).

[SergeyBiryukov]

Could we bump the priority for this? See the discussion in ​https://twitter.com/Josh412/status/1100487567236059136 and ​https://twitter.com/Rarst/status/1100813140726562816.

Also discussed with @Rarst at WordCamp Nordic 2019.

[Rarst]

Do note that stable PHP is at 7.3 by now. In larger PHP community it seems that 7.1 is going to be "common" minimum required version for a while.

Having a hard barrier to use current (not even new anymore) PHP features significantly impairs progress of plugin ecosystem. While back I had counted barely 400+ PHP 7+ plugins. This is abysmally low.

[dd32]

Latest system request for this: ​https://make.wordpress.org/systems/2019/03/18/change-plugins-svn-linting-to-php-7-current-please/

I've requested that we continue to bump it as we roll out new PHP branches to dotorg, however due to the way the dotorg infrastructure is setup that might not be as easily achieved.

[Otto42]

We have had problems with the current iteration of the php linter, insofar as it looks at each individual file independently.

If a plugin includes backwards compatibility files in a way where the back compat checks are actually in another file, then the non compatible file, which normally would not be loaded in such an environment, will fail the lint check and thus cannot be committed.

Example: plugin does if ! function exists, then include file. File has that function. Function is a back commit function that exists in PHP 7. File, by itself, fails the lint. But it's never loaded because the plugin doesn't load it in such a case.

This is shockingly common in libraries included by plugins, so the authors don't know why it fails, because it's a library they didn't write.

[danielbachhuber]

I'm getting this error when I try to commit to solr-power:

svn: E165001: Commit failed (details follow):
svn: E165001: Commit blocked by pre-commit hook (exit code 1) with output:

***********************************
PHP error in: solr-power/trunk/vendor/symfony/contracts/HttpClient/ChunkInterface.php:

Parse error: syntax error, unexpected '?' in solr-power/trunk/vendor/symfony/contracts/HttpClient/ChunkInterface.php on line 65
Errors parsing solr-power/trunk/vendor/symfony/contracts/HttpClient/ChunkInterface.php

It looks like there might be some PHP 7.3 syntax in the Symfony dependency. Is there a way to skip the pre-commit hook?

[dd32]

Replying to danielbachhuber:

Is there a way to skip the pre-commit hook?

Unfortunately not.

[Ipstenu]

Is there anyone we can poke about this? It's getting progressively worse and more and more libraries people use are PHP 7.1+

Plus now that we block new submissions until people have used their already approved plugins, we get developers stuck :( The only work-around I can do for them is close the plugin and let them continue on, but it's a terrible experience.

[jvanhengeldationnl]

Same here. Checking in our plugin is blocked for all our libraries use 7.1+.

[dhuethorst]

Any updates? Blocked by some libraries using 7.1...

[danielbachhuber]

@dd32 @Otto42 Is there a known timeline for this being fixed? It would be concerning if this started blocking security updates to plugins (for plugins dependent on a third-party library).

[dd32]

Replying to danielbachhuber:

Is there a known timeline for this being fixed?

There's no timeline, it'll be fixed when the systems team gets to it and it's reasonable.

[danielbachhuber]

For the next person to run into this issue with their Composer dependencies, I thought of a temporary workaround:

"config": {
    "platform": {
        "php": "7.0"
    }
}

If you haven't seen this before, this tells Composer to behave as though it's updating your dependencies on a PHP 7.0 system. For us, the only bundled change ended up being Downgrading symfony/event-dispatcher (v4.2.8 => v3.3.6), so it's a reasonable compromise. However, I did have to also "phpunit/phpunit": "^6 || ^7".

Lastly, I found ​https://github.com/JakubOnderka/PHP-Parallel-Lint to be a neat tool for seeing which files failed to lint. If you're on a Mac ​here are instructions on how to install PHP 7.0 via Homebrew. Once you've done so, linting looks like this:

parallel-lint ./ -p /usr/local/Cellar/php@7.0/7.0.33/bin/php

[xedin.unknown]

For the next person to run into this issue with their Composer dependencies, I thought of a temporary workaround:

"config": {
    "platform": {
        "php": "7.0"
    }
}

This is not a work-around, because it prevents using code that requires any version that is higher (including higher patches, e.g. 7.0.1)

IMO it is completely unreasonable to prevent commits if they fail to lint at any single PHP version, and the only acceptable solution is to make linting warn you, instead of outright rejecting the commit. If bumped up to 7.0, there definitely can be PHP 5.6 code which will start to behave unpredictably or outright break due to ​BC breaking changes. Same will happen if bumped to 7.1. There is simply no one single linter target version that can satisfy the whole range of plugins that are acceptable in the SVN repository. The current restriction is stopping the community from using faster, safer, more robust and readable code, not to mention other libraries and standards, like the PSR-14. It is an obstacle for open-source software.

Please remove the requirement for successful linting. We are an enterprize, and as such most of our plugins use at least PHP 7.1.

[ayeshrajans]

I would like to +1 what @xedinunknown said too. We can now mention a PHP version requirement in readme.txt - If this is set, I propose we use a PHP binary of that version or not run the linter at all.

[dd32]

Sorry for the delay everyone, the PHP lint checker has been increased to PHP 7.2.

The version will be increased in the future when we upgrade what we're using on other WordPress.org servers, as @stankea ​said on the system request:

The server was using the Debian’s latest version (which is 7.0 on that server). I’ve applied the nginx-php role (which also includes cli) we use on other wporg web servers, so it’s running 7.2.3 now, and should use newer version if/when we update the wporg web servers php version.

[Rarst]

Sorry for the delay everyone, the PHP lint checker has been increased to PHP 7.2.

I appreciate the progress, but I urge to reconsider if the approach is optimal.

7.2 was released almost two years ago.

Is it an improvement over 7.0? Yes.

Is it an adequate PHP version to make a hard ceiling for repository plugins? No.

Is the "whatever server happens to have" adequate policy to manage what is allowed in the repo? No.

[dd32]

Replying to Rarst:

I appreciate the progress, but I urge to reconsider if the approach is optimal.

7.2 was released almost two years ago.

I understand where you're coming from, but IMHO running the branch prior to current stable is far more than ideal than either running PHP 7.0 linting or no linting at all.

Given PHP's poor adoption curves, all that this means is that you can't release a plugin through WordPress.org that only works on 15% of WordPress sites.

You can still release a plugin that relies upon PHP 7.2 code, but at least that'll be usable by ~45% of sites instead.
You can even use newer functions in PHP if including compat code, just not newer syntax.

As new PHP branches are deployed for usage on WordPress.org itself, the version that plugins.svn.wordpress.org will accept will increase in tandem.

[xedin.unknown]

Like I wrote before, and with agreement with @Rarst, this is hardly a solution, because it still sets a hard ceiling to an arbitrary version of PHP. The only acceptable solution IMO is to make the check optional. This means that people who require hard compatibility with WP.org can still have that, and everyone can still be informed if their application has problems, but nevertheless it can and should be the choice of product developers which PHP version to support.

[giuseppe.mazzapica]

Replying to dd32:

I understand where you're coming from, but IMHO running the branch prior to current stable is far more than ideal than either running PHP 7.0 linting or no linting at all.

More ideal than running PHP 7.0 linting for sure. Andrey already said that quite clearly.

Better than no linting at all, not so sure. At least if by "linting" we mean running php -l.

PHP parser (​https://github.com/nikic/PHP-Parser) runs on PHP 7.0+ and it is able to parse from PHP 5.2 to the most recent version of PHP and, among other things, detect errors. It is reliable and kept up-to-date, considering its maintainer is one of the most active core PHP developers.

Using PHP parser to check code would allow to check syntax for versions more recent than the running version, and could also provide a better feedback in case of failure.

On top of that, it would be possible to implement an automatic detection of the minimum required PHP version, something very useful now that WordPress is able to prevent plugin installation in case user's PHP version is too old.

Not to mention the possibility to use it to obtain interesting statistics about PHP code used by plugins, that could be used to inform future decisions or the possibility to switch to a PHP-7-only parser when WP would abandon PHP 5.

Which means there's here room to improve both developers' and users' experience.

[uglyrobot]

In my case the linter is preventing forward compatibility with PHP8 in my plugin trying to use the latest AWS SDK via Composer.

PHP error in: infinite-uploads/tags/1.0/vendor/symfony/polyfill-mbstring/bootstrap80.php:
Parse error: syntax error, unexpected '|', expecting variable (T_VARIABLE) in Standard input code on line 15

The bootstrap80.php file is only included by this lib in PHP8+ and has PHP8 only syntax, so it's not a backwards compatibility issue (plugin and composer library works all the way back to 5.5) but a forward compatibility issue. As you can see this is within a composer dependency so I don't really have much control over it. I want my plugin to work in PHP8+. For now i've been forced downgrade that package but that's a security issue potentially running older code.

We really do need to find a better way to lint code or a way to bypass it. Right now it's discouraging forward compatibility, not just forcing backward compatibility!

[pomegranate]

@uglyrobot until this is fixed, I think the most simple solution is to patch the affected files in your deployment process (mine is automated by a github action).
So after fetching my composer libraries I simply execute:

sed -i "s/: string|false//" "/vendor/symfony/polyfill-intl-idn/bootstrap80.php"
sed -i "s/: string|false//" "/vendor/symfony/polyfill-intl-normalizer/bootstrap80.php"

(which in this case removes the return type declaration that is not strictly necessary)

[liedekef]

I've been blocked on multiple occasions by the pre-commit linting. I include some payment provider libs in my plugin (mercadopago, mollie, ...) and sometimes they check which version of php is used and include other files based on that. However I'm not going to reverse engineer those each time ... and removing php 8 files (if I could even find those) would create problems for those people actually wanting to test/run php 8.
So I urge you to also make this linting optional.

[leonstafford]

+1 for making linting optional.

Based on the speed of this ticket's advancement, I don't have hope for linting which matches the plugin's "min PHP" setting coming into effect.

I pulled one plugin off wp.org last year, partly due to this annoyance. Considering to add a new one, which has a PHP 7.4 minimum requirement, so waiting more years just to get the next bump to 7.3 won't work.

This is an unnecessary impediment to improving modern PHP adoption within WP.

[dd32]

Linting is currently PHP 7.4 I believe.

I'm not sure on the timeline / what's preventing / etc WordPress.org migrating to PHP8, I suspect it's simply time restrictions.

[marekdedic]

Hi,
where can we find the version used by the pre-commit hook please? I'd like to update my code to PHP 8, but I don't know if/when I can safely do that...

Thanks

[SergeyBiryukov]

Replying to marekdedic:

where can we find the version used by the pre-commit hook please?

As noted above, the pre-commit handler is currently using PHP 7.4, here's the latest Systems request:
​https://make.wordpress.org/systems/2020/09/10/hi-can-we-please-check/

[ttodua]

What about people using regular up-to-date version of php, i.e. 8.0 + ? I don't think pedalizing the support of new versions in favor of obsolete php versions, was the right choice.

[dd32]

Replying to ttodua:

What about people using regular up-to-date version of php, i.e. 8.0 + ?

We're currently limited to using whatever version of PHP is deployed in WordPress.org production, currently that's PHP 7.4, but PHP8 is on a roadmap. There's no current specific timeframe.

PHP8.0+ currently accounts for < 6.5% of recent WordPress installs (ie. WordPress 5.7+) and < 7.5% of the latest version of WordPress.

As such, there's no general benefit for WordPress users (NOT plugin developers) to either a) remove the PHP lint checking, or b) rush the systems team on deploying PHP8 before they're comfortable switching production over to it.

(edit: I realise that this is also preventing forward-compatibility for some, there's not much we can do about that at this time, and wasn't the point of the last comment I was responding to)

[liedekef]

The fact that the linting prevents any real php 8 code is preventing a lot of up to date libraries to be included. Many libraries have built-in checks if they run on php 7 or 8, and svn commit is failing because it looks at the codebase, not the logic.
What is the point of even releasing a post like ​https://make.wordpress.org/core/2020/11/23/wordpress-and-php-8-0/ if no plugin can contain php 8 code?

I do understand the case for wordpress users, but plugin developers are getting limited every time a php release is done. Even the readme tag "Requires PHP: 8.0" won't be of much use I assume ...

[ttodua]

I agree to @liedekef, I don't see any point in preventing developers to use whatever PHP version is officially released. Our plugins have their requirements and etc, where we define and tell users who can use the plugin. If there are special plugin, which we decide to share to publish , because of our good will - but that plugin only works for PHP 8 -then you should allow: whoever wants to use that plugin and is willing to setup php 8 on their servers - let them do so. Why pedalizing them for the sake of other people who use php 5.
idk, but the solution might be to fix the linter

[zodiac1978]

After reading all the comments I am wondering why the idea from @giuseppemazzapica has got no replies. This seems to solve the problem of being bound to the PHP version from the underlying system.

Why has nobody tried to go this way? Is there something blocking this path?

[dd32]

PHP Linting has been upgraded to PHP 8.1.

Switching to a different PHP linter isn't something that can be reasonably done right now, due to implementation details.

I'm hopeful we'll be able to upgrade the PHP linting binary within a more reasonable timeframe now though.