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

restart phpcs/phpcbf when xdebug is loaded #3724

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

staabm
Copy link

@staabm staabm commented Dec 5, 2022

closes #3337

running phpcs on the project itself takes 11-12 seconds when xdebug is loaded.
with the added restart script it takes 6-7 seconds. so its roughly 40-50% faster.

running phpcs with a primed cache takes ~1.8 seconds without this PR and ~0.9 seconds with the PR.

test on mac m1 pro

php -v
PHP 8.1.13 (cli) (built: Nov 24 2022 15:58:42) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.13, Copyright (c) Zend Technologies
    with Xdebug v3.1.5, Copyright (c) 2002-2022, by Derick Rethans
    with Zend OPcache v8.1.13, Copyright (c), by Zend Technologies

@staabm staabm marked this pull request as ready for review December 5, 2022 20:38
Copy link
Contributor

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very much like the principle of this change, however I do see some potential problems.

  1. I know this project is not keen on adding additional run-time dependencies.
    If this would be accepted, I believe the script to generate the PHAR file - scripts/build-phar.php - will need to be adjusted too.
    I also think that this will probably not play nice with a PEAR install, which this package still supports, though PEAR support will be dropped in PHPCS 4.x.
  2. Loading the vendor/autoload.php file may have side-effects for the functioning of certain sniffs.
    Most sniffs are written to presume a "clean" environment with only PHPCS loaded in memory.
    Loading the vendor/autoload.php file can result in other packages loading files into memory, especially packages which are intended as polyfills have a habit of doing this.
    Any sniff which includes a presumption of a "clean" environment may now start to throw false positives/negatives if they, for instance, use function_exists() or class_exists() to check for PHP native functions versus userland functions - or when they want to flag PHP functions not available on the PHP version on which PHPCS is being run.

What could help, would be if the xdebug-handler package contained its own autoloader and to only include that autoloader instead of including vendor/autoload.php. Still not sure if that's enough though.

What would be even better, would be a PHPCS native implementation of the same, though I haven't looked in detail at the package to know how much work (and code) would be involved in that.

Either way, just my two pennies.

@staabm
Copy link
Author

staabm commented Dec 6, 2022

2. Loading the vendor/autoload.php file may have side-effects for the functioning of certain sniffs.

the external package added consists only of 4 classes. we could include these 4 files manually so we don't need to bootstrap the whole autoloader.

  1. If this would be accepted, I believe the script to generate the PHAR file - scripts/build-phar.php - will need to be adjusted too.

thanks for the hint. I can try to take care of it, if required.

@staabm
Copy link
Author

staabm commented Dec 26, 2022

Ping @gsherwood

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codesniffer slow when Xdebug is enabled
2 participants