Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#7997 closed defect (bug) (fixed)

PHP version compatibility sniffs

Reported by: boonebgorges's profile boonebgorges Owned by: netweb's profile netweb
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch commit
Cc:

Description

Previously, we introduced PHP linting into our build/test process: #7926.

Leveraging php -l is a good start for catching blatant syntax errors, but it's very limited in terms of what it catches. Removed functions and invalid (but syntactically correct) constructs are a few examples of things that can only be caught by tokenized analysis. See eg #7925.

I propose that we switch to using PHPCS with the PHPCompatibilityWP sniffs. This change provides us with dramatically better coverage of deprecated usages, on a per-PHP-version basis. It's sensitive to WP's polyfills (for functions like hash_equals()). And it combines all of the version compatibility checks into a single run, so that the checks only need to be run once in the Travis matrix (instead of separately on each version of PHP).

The incoming patch shows an implementation of this. It does the following:

  • Installs phpcompatibility-wp and its dependencies via Composer
  • Adds a Grunt task exec:phpcompat. (There's a grunt-phpcs package, but it doesn't let you pass --runtime-set flags.) I called it phpcompat because we will likely want to introduce a phpcs task in the future for coding standards more generally.
  • Adds exec:phpcompat to the grunt commit task, replacing the now-redudant phplint
  • Adds a Grunt task travis:phpcompat task, paralleling travis:codecoverage and travis:grunt, so that all PHP compatibility checks happen in their own job

To run locally, composer update (or install) && npm update, then grunt exec:phpcompat. To see in action on TravisCI,
https://travis-ci.org/boonebgorges/BuddyPress (Note that we don't currently pass! That'll be the subject for another ticket.)

Attachments (1)

7997.diff (31.5 KB) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (11)

@boonebgorges
6 years ago

#1 @netweb
6 years ago

  • Keywords commit added

Nice one, tested and works for me 👍

#2 @boonebgorges
6 years ago

In 12277:

Introduce PHP compatibility build/test tools.

The new exec:phpcompat Grunt task uses the phpcompatibility-wp library
to do a WordPress-sensitive check for incompatibilities between our codebase
and the PHP versions that we support.

A new Travis CI job is dedicated to running the phpcompat tests. This
pattern parallels our travis:grunt job.

The new exec:phpcompat tests supercede the phplint task introduced in #7925.

See #7997.

#3 @boonebgorges
6 years ago

  • Milestone changed from Up Next to 4.0

Thanks for having such a quick look, @netweb !

#4 @boonebgorges
6 years ago

Sorry, botched that commit - fix coming up.

#5 @boonebgorges
6 years ago

In 12278:

Another try at adding exec:phpcompat Grunt task.

Changeset [12277] didn't properly apply the changes.

See #7997.

#6 @boonebgorges
6 years ago

In 12282:

Travis CI: Don't install Composer packages on PHP < 5.4.

The phpcompatibility-wp library has requirements that need at least PHP 5.4,
and we only use this library when testing against 7.2, so there's no need
to install on earlier version of PHP.

This will likely have to be revisited at some point in the future, when
we use Composer for more integral parts of the build/dev process.

See #7997.

#7 @boonebgorges
6 years ago

  • Resolution set to fixed
  • Status changed from new to closed

This is now working, with the caveat that the Composer requirements mean that composer install now fails on PHP 5.3. And since it's only for require-dev, it likely won't make a difference for anyone using Composer to pull into a project. I'm going to mark this as closed, with the note that we should revisit some aspects of the current setup when we drop PHP 5.3 support.

#8 @jrf
5 years ago

Just came across this ticket.

While I - of course - fully support adding PHPCompatibilityWP to the QA toolset, it is not a replacement for linting code.

When PHPCompatibility encounters parse errors which are not typically to do with PHP cross-version compatibility, it will ignore them and not notify you of these.

Also, in case of parse errors, the results of PHPCompatibility may not be reliable as the file may be tokenized incorrectly by PHP/PHPCS because of the parse errors.

I'd strongly recommend using php -l against at least the lowest and highest PHP version, if not lowest/highest of each PHP major in addition to using PHPCompatibility.

#9 @boonebgorges
5 years ago

In 12424:

Reintroduce PHP linting to automated QA.

See #8124, #7997.

#10 @boonebgorges
5 years ago

Thanks for checking in, @jrf ! See #8124.

Note: See TracTickets for help on using tickets.