-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Implement GH-10024: support linting multiple files at once using php -l #10710
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
Conversation
b765aaa to
feb4eec
Compare
Thank you for working on it 🙏.
Could you give an indication what the perf difference is linting e.g. 2 or 3 (or a few) files in a row vs. in a single process?
The thesis of the initial issue was, that it should be way faster.
Sure.
For the root folder of Wordpress (15 files) I get 0.013s for linting them all at once, and I get 0.154s when linting them one by one.
For two files I get 0.009s for linting them all at once, and 0.018s for linting them one by one. It's not a coincidence that it's twice as much for linting one by one because the overhead outweighs everything else in this case.
So yeah, it's better for performance.
Can this PR land in PHP 8.2.x or can such a change only be done in 8.3.x
Can this PR land in PHP 8.2.x or can such a change only be done in 8.3.x
It's a new feature so unfortunately you'll have to wait as this will land in 8.3
@nielsdos any chance this could slip into 8.3?
@nielsdos any chance this could slip into 8.3?
I've re-requested a review.
Anyone else could review the changes? Would be great this PR could be part of PHP 8.3, as it is the foundation for speeding up php linting tools which can potentially save a lot of CI time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, thanks @nielsdos!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this comes from lines 2457-2460. Either we can change the error message for CGI in general, or special case the linting case. I think changing the message in general might be dangerous for BC though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This differs in behavior with CLI, in that it aborts when a file is not found. Is this a limitation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we'd need to change error handling in lines 2451-2488: if a file cannot be opened or accessed CGI aborts. But I'm not sure if we should special case it because it will make things more complicated and possibly slow down regular use of CGI. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt linting is often used with php-cgi, so this should be ok.
...hp -l This is supported in both the CLI and CGI modes. For CLI this required little changes. For CGI, the tricky part was that the options parsing happens inside the loop. This means that options passed after the -l flag were previously simply ignored. As we now re-enter the loop we would parse the options again, and if they are handled but don't set the script name, then CGI will think you want to read from standard in. To keep the same "don't parse options" behaviour I simply wrapped the options handling inside an if.
9771130 to
01b289d
Compare
thank you so much to anyone who made this possible!!
Closes GH-10024
This is supported in both the CLI and CGI modes. For CLI this required little changes.
For CGI, the tricky part was that the options parsing happens inside the loop. This means that options passed after the -l flag were previously simply ignored. As we now re-enter the loop we would parse the options again, and if they are handled but don't set the script name, then CGI will think you want to read from standard in. To keep the same "don't parse options" behaviour I simply wrapped the options handling inside an if.