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

Manually load comments if opcache.save_comments disabled. #4161

Merged
merged 3 commits into from Jun 19, 2017

Conversation

2 participants
@gitlost
Contributor

gitlost commented Jun 18, 2017

Issue #4160

Adds loading of source files and getting their doc comments if opcache.save_comments is disabled.

Adds an entry in the travis matrix with env var SAVE_COMMENTS_DISABLED which is then checked in ci/test.sh to call the behat tests with WP_CLI_PHP_ARGS set appropriately, and changes FeatureContext::get_process_env_variables() to pass this on.

Adds a new tag @require-opcache-save-comments to skip a test which uses wp-cli 1.1.0 (which can't deal with opcache.save_comments disabled).

(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.)

@danielbachhuber

This is quite good, @gitlost. Thanks for your work on it. Just a few small nits.

Show outdated Hide outdated .travis.yml
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 );

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 19, 2017

Member

We should use PHP_EOL over \n

@danielbachhuber

danielbachhuber Jun 19, 2017

Member

We should use PHP_EOL over \n

This comment has been minimized.

@gitlost

gitlost Jun 19, 2017

Contributor

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.

@gitlost

gitlost Jun 19, 2017

Contributor

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.

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 19, 2017

Member

We use PHP_EOL consistently elsewhere, so we should use it in this case too.

@danielbachhuber

danielbachhuber Jun 19, 2017

Member

We use PHP_EOL consistently elsewhere, so we should use it in this case too.

This comment has been minimized.

@gitlost

gitlost Jun 19, 2017

Contributor

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.

@gitlost

gitlost Jun 19, 2017

Contributor

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.

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 19, 2017

Member

Ok. We can call this debate still undecided for now.

@danielbachhuber

danielbachhuber Jun 19, 2017

Member

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() ) ) );

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 19, 2017

Member

Ditto PHP_EOL

@danielbachhuber

danielbachhuber Jun 19, 2017

Member

Ditto PHP_EOL

@danielbachhuber danielbachhuber added this to the 1.3.0 milestone Jun 19, 2017

@danielbachhuber danielbachhuber merged commit 12c24ce into master Jun 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber deleted the issue_4160 branch Jun 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment