Skip to content

Don’t compile when Opcache is not enabled on CLI #20883

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

Closed
wants to merge 2 commits into from

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Dec 12, 2016

Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #20878
License MIT

This should fix #20878 "Zend OPcache seems to be disabled, can't compile file" when Opcache is enabled, but opcache.enable_cli is turned off.

@@ -109,15 +109,28 @@ protected function doSave(array $values, $lifetime)
$value = serialize($value);
}
} elseif (!is_scalar($value)) {
throw new InvalidArgumentException(sprintf('Value of type "%s" is not serializable', $key, gettype($value)));
throw new InvalidArgumentException(sprintf('Value of type "%s" is not serializable', gettype($value)));
Copy link
Member

Choose a reason for hiding this comment

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

Borrowed from PhpArrayAdapter:
throw new InvalidArgumentException(sprintf('Cache key "%s" has non-serializable %s value.', $key, gettype($value)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

*/
private function compileFile($file)
{
if ('cli' === php_sapi_name() && !ini_get('opcache.enable_cli')) {
Copy link
Member

Choose a reason for hiding this comment

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

Use PHP_SAPI constant instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

/**
* @param string $file
*/
private function compileFile($file)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure the extra method is worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removed it.

*/
private function compileFile($file)
{
if ('cli' === php_sapi_name() && !ini_get('opcache.enable_cli')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be changed to an or-case with the @opcache_compile_file($file);

if ('cli' === PHP_SAPI || !ini_get('opcache.enable_cli')) {
    @opcache_compile_file($file);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just typed these lines, haha :)

@fabpot
Copy link
Member

fabpot commented Dec 13, 2016

Thank you @ruudk.

fabpot added a commit that referenced this pull request Dec 13, 2016
This PR was squashed before being merged into the 3.2 branch (closes #20883).

Discussion
----------

Don’t compile when Opcache is not enabled on CLI

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20878
| License       | MIT

This should fix #20878 "Zend OPcache seems to be disabled, can't compile file" when Opcache is enabled, but `opcache.enable_cli` is turned off.

Commits
-------

5222643 Don’t compile when Opcache is not enabled on CLI
@fabpot fabpot closed this Dec 13, 2016
@fabpot fabpot mentioned this pull request Dec 13, 2016
@nicolas-grekas
Copy link
Member

@ruudk ruudk deleted the fix-opcache-cli branch December 13, 2016 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants