Implement GH-10024: support linting multiple files at once using php -l #10710
Conversation
|
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 |
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? |
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 |
| Array | ||
| ( | ||
| [0] => No syntax errors detected in %s012_good.test.php | ||
| [1] => No input file specified. |
There was a problem hiding this comment.
This error is a bit confusing.
There was a problem hiding this comment.
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.
| Array | ||
| ( | ||
| [0] => No syntax errors detected in %s012_good.test.php | ||
| [1] => No input file specified. |
There was a problem hiding this comment.
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.
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.
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.
|
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.