Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.0 | Refactor the requirements check #530

Open
jrfnl opened this issue Jul 2, 2024 · 0 comments
Open

4.0 | Refactor the requirements check #530

jrfnl opened this issue Jul 2, 2024 · 0 comments

Comments

@jrfnl
Copy link
Member

jrfnl commented Jul 2, 2024

Current Situation

Currently PHPCS has two entry points: the phpcs and phpcbf bootstrap files in the bin directory.

Those two files have a similar content:

  • Include the autoloader.
  • Instantiate a PHP_CodeSniffer\Runner object.
  • Run the relevant method in the Runner class (either runPHPCS() or runPHPCBF()).
  • Handle the exiting with an exit code.

Within the Runner methods, PHPCS subsequently:

  • Has some guard code for out of memory issues.
  • Starts the timer.
  • Does a requirements check
  • Starts the actual run with everything that comes with it.

Why this is problematic

PHPCS 3.x has a minimum supported PHP version of 5.4.
PHPCS 4.x has a minimum supported PHP version of 7.2 and the intention is to selectively modernize the PHPCS codebase to start using features introduced in PHP 5.5 - 7.2.

As part of the requirements check, PHPCS checks the runtime PHP version complies with the minimum supported PHP version.

To allow for this check to run without causing fatal/parse errors before it can alert the end-user of a problem with the minimum supported PHP version, all files loaded up to that point and which are involved in that check (bootstrap files, Autoloader, Runner, DeepExitException classes (as a minimum)) need to be fully PHP cross-version compatible.

As things are at the moment, this means that any file involved in the minimum requirements check cannot be modernized/cannot use PHP 5.5 - 7.2 features as that would cause a parse error, which would break the minimum requirements check before it can alert the end-user of a problem.

Solution

To prevent these type of fatals and still allow for modernizing the codebase with only minimal exceptions, I intend to:

  • Isolate the code for the minimum requirements check into a separate file/class.
  • Run the minimum requirements check earlier - from within the bootstrap files, before loading the autoloader/any other PHPCS files.

If done correctly, this would effectively only leave the following files which need to be fully PHP cross-version compatible:

  • The bootstrap files.
  • The code for the requirements check.
  • The DeepExitException class.

The files which need to stay PHP cross-version compatible (minimum PHP version 5.0) should get a clear warning near the top of the file to mark them as such.

Additionally, in the GitHub actions workflows, safeguards should be put in place to:

  1. Test that the requirements check works as intended on PHP < 7.2 and PHP 7.2+.
  2. Safeguard that the relevant files involved in the requirements check are PHP cross-version compatible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant