Manually load comments if opcache.save_comments disabled. #4161
Conversation
danielbachhuber
left a comment
danielbachhuber
left a comment
There was a problem hiding this comment.
This is quite good, @gitlost. Thanks for your work on it. Just a few small nits.
.travis.yml
Outdated
| - php: 5.3 | ||
| env: WP_VERSION=3.7.11 DEPLOY_BRANCH=master | ||
| - php: 7.1 | ||
| env: WP_VERSION=latest SAVE_COMMENTS_DISABLED=1 |
There was a problem hiding this comment.
Rather than spawn a full new job for this, which will add substantial time to the overall build, can we instead put this into the existing PHP 7.1 job (possibly as a second Behat run which only runs @require-opcache-save-comments)?
There was a problem hiding this comment.
That was actually the way I tried it first off, but it makes the PHP 7.1 job very long, whereas there's some parallelism doing it separately (and you get early failure in the first key case) - but it's much of a muchness - I'll put it into the existing job and see what you think.
There was a problem hiding this comment.
it makes the PHP 7.1 job very long
Only running the one scenario for this test?
There was a problem hiding this comment.
No, that was running the whole thing with save_comments disabled, sorry missed that. Do you mean just run it on a subset of the features? (The tag @require-opcache-save-comments is only to avoid a test.)
There was a problem hiding this comment.
Do you mean just run it on a subset of the features? (The tag
@require-opcache-save-commentsis only to avoid a test.)
Yes — you can use the tag mentioned to only run that test.
There was a problem hiding this comment.
Sorry for the confusion. To better communicate, I created a PR with my suggested changes: #4163
Travis isn't running for some reason; I think because it's a PR against a branch.
There was a problem hiding this comment.
@gitlost Sorry for even more confusion. I think the PHPUnit tests are sufficient for this change. We don't need to make modifications to the Behat test suite, or run Behat tests specific to this change.
There was a problem hiding this comment.
Cool, I'll remove the job - that only leaves the Great Newline Debate..
There was a problem hiding this comment.
I left the tag in the "bootstrap.feature" test as a sort of reminder/comment. That and the passing of WP_CLI_PHP_ARGS in FeatureContext::get_process_env_variables() are unnecessary now - should I remove them?
There was a problem hiding this comment.
Nah, those are fine as is.
| if ( isset( self::$file_contents[ $filename ] ) ) { | ||
| $contents = self::$file_contents[ $filename ]; | ||
| } elseif ( is_readable( $filename ) && ( $contents = file_get_contents( $filename ) ) ) { | ||
| self::$file_contents[ $filename ] = $contents = explode( "\n", $contents ); |
There was a problem hiding this comment.
We should use PHP_EOL over \n
There was a problem hiding this comment.
I'm thinking though as long as a "\n" is in the EOL, then this is safer - unless the files are Mac formatted circa 1990s with a single "\r"! If it is Windows "\r\n" then the existing code is agnostic to that, so works regardless of file format.
There was a problem hiding this comment.
We use PHP_EOL consistently elsewhere, so we should use it in this case too.
There was a problem hiding this comment.
It depends on the context though. I found 4 cases where PHP_EOL is being used in similar contexts (ie parsing a file): https://github.com/wp-cli/wp-cli/blob/master/features/steps/given.php#L75, https://github.com/wp-cli/wp-cli/blob/master/features/bootstrap/support.php#L146, https://github.com/wp-cli/wp-cli/blob/master/php/WP_CLI/DocParser.php#L158 and https://github.com/wp-cli/wp-cli/blob/master/php/WP_CLI/Dispatcher/Subcommand.php#L279 - in all 4 cases it would be better (ie more portable) to use "\n" instead of PHP_EOL - as is done for instance in get_longdesc() https://github.com/wp-cli/wp-cli/blob/master/php/WP_CLI/DocParser.php#L56.
There was a problem hiding this comment.
Ok. We can call this debate still undecided for now.
| return null; | ||
| } | ||
|
|
||
| return self::extract_last_doc_comment( implode( "\n", array_slice( $contents, 0, $reflection->getStartLine() ) ) ); |
2 participants
Issue #4160
Adds loading of source files and getting their doc comments if
opcache.save_commentsis disabled.Adds an entry in the travis matrix with env var
SAVE_COMMENTS_DISABLEDwhich is then checked inci/test.shto call the behat tests withWP_CLI_PHP_ARGSset appropriately, and changesFeatureContext::get_process_env_variables()to pass this on.Adds a new tag
@require-opcache-save-commentsto skip a test which uses wp-cli 1.1.0 (which can't deal withopcache.save_commentsdisabled).(Also there's an unused function
CommandFactory::clear_file_contents_cache()which could be useful to save memory usage but it's difficult to know when to call it given the dynamic autoloading.)