The current check introduced in #2502507: Add a hook_requirements() warning if no opcode cache is enabled is too demanding, and as a consequence, doesn't allow to use more secure settings, like opcache.restrict_api protection, because it will in turn auto-disable opcache_get_status(). We should instead use the check Symfony already has in place:

'zend_opcache_enabled' => extension_loaded('Zend OPcache') && ini_get('opcache.enable'),

It can be used via public function PhpConfigDataCollector::hasZendOpcache

In fact, we could even re-use the same simple and reliable check we already have for the only opcache function used in core:

if (function_exists('opcache_invalidate')) {}

This allows to configure any restrictions the server admin wants to avoid data leaks (like paths to scripts between sites on the same system) which are available if you allow opcache_get_status() to satisfy Drupal current requirements.

Any change is required in system requirements and install.php.

CommentFileSizeAuthor
#25 opcache-ok.jpg184.49 KBmemtkmcc
#23 2639878.patch2.64 KBrobloach
#21 2639878-opcache-8.2.x.patch2.64 KBrobloach
#17 2639878-opcache-8.2.x.patch2.73 KBrobloach
#14 2639878-opcache-check-8.2.patch1.61 KBrobloach
#13 2639878-opcache-check.patch1.61 KBrobloach
#9 0001-Issue-2639878-Use-less-demanding-check-to-see-if-OPc.patch1.62 KBomega8cc
#4 0001-Issue-2639878-by-omega8cc-Use-less-demanding-check-t.patch1.67 KBomega8cc

Comments

omega8cc created an issue. See original summary.

omega8cc’s picture

omega8cc commented
Related issues: +#2421451: Drupal needs comments in opcache

It should also affect similar check introduced in #2421451: Drupal needs comments in opcache

cilefen’s picture

cilefen commented
Issue summary: View changes
omega8cc’s picture

omega8cc commented
StatusFileSize
new 0001-Issue-2639878-by-omega8cc-Use-less-demanding-check-t.patch1.67 KB

A patch for review attached.

omega8cc’s picture

omega8cc commented
Status: Active » Needs review

Status update.

cilefen’s picture

cilefen commented 
+++ b/core/install.php
@@ -26,7 +26,7 @@
-if (function_exists('opcache_get_status') && opcache_get_status()['opcache_enabled'] && !ini_get('opcache.save_comments')) {
+if (function_exists('opcache_invalidate') && !ini_get('opcache.save_comments')) {

Is it possible checking for opcache_invalidate isn't good enough? I seem to remember the function can exist without opcache being really enabled.

omega8cc’s picture

omega8cc commented

Yes, we can add the extra ini check, but I also have to fix the logical error I have introduced.

omega8cc’s picture

omega8cc commented

Note that everywhere we use opcache_invalidate() in core we don't check for anything else, so maybe we should extend the check also elsewhere to match the logic used in Symfony, but it should be a separate issue then, I think?

omega8cc’s picture

omega8cc commented
StatusFileSize
new 0001-Issue-2639878-Use-less-demanding-check-to-see-if-OPc.patch1.62 KB

Better patch attached for review.

cilefen’s picture

cilefen commented
Issue tags: +Performance

The last submitted patch, 4: 0001-Issue-2639878-by-omega8cc-Use-less-demanding-check-t.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

robloach’s picture

robloach
he/him
commented
StatusFileSize
new 2639878-opcache-check.patch1.61 KB

This issue is persistent in PHP7, even though opcode comes with PHP7 by default. I've updated the patch to make sure there aren't any git conflicts.

robloach’s picture

robloach
he/him
commented
Version: 8.1.x-dev » 8.2.x-dev
Category: Task » Bug report
StatusFileSize
new 2639878-opcache-check-8.2.patch1.61 KB

And here it is for Drupal 8.2.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Version: 8.2.x-dev » 8.1.x-dev
Status: Needs review » Reviewed & tested by the community

This seems like a reasonable change for 8.1.x as a bug report.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Reading the docs...

opcache.restrict_api string
Allows calling OPcache API functions only from PHP scripts which path is started from specified string. The default "" means no restriction.

I don't see how using the existence of the opcache_invalidate() is any different from using opcache_get_status(). How about just adding a static method to \Drupal\Component\Utility\OpCodeCache called isEnabled that just checks return extension_loaded('Zend OPcache') && ini_get('opcache.enable'); and then using this in the two locations. I don't think we need to use PhpConfigDataCollector that does far more than we need it too.

robloach’s picture

robloach
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new 2639878-opcache-8.2.x.patch2.73 KB

Moves to an OpCodeCache::isEnabled() function, also touched it up to use opcache_get_status().

Updated the PR at https://github.com/RobLoach/drupal/pull/8 . Applies cleanly against 8.1.x and 8.2.x.

Status: Needs review » Needs work

The last submitted patch, 17: 2639878-opcache-8.2.x.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review

Looks like unrelated migrate test failures

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@RobLoach why not check with the code Alex mentioned?

return extension_loaded('Zend OPcache') && ini_get('opcache.enable');

The cleanup with the static is nice btw!

robloach’s picture

robloach
he/him
commented
StatusFileSize
new 2639878-opcache-8.2.x.patch2.64 KB

Missed that one, thanks.

Status: Needs review » Needs work

The last submitted patch, 21: 2639878-opcache-8.2.x.patch, failed testing.

robloach’s picture

robloach
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new 2639878.patch2.64 KB

Updated.

omega8cc’s picture

omega8cc commented

@alexpott -- re: "I don't see how using the existence of the opcache_invalidate() is any different from using opcache_get_status()" -- I have explained this at the top of this issue -- you can't use opcache_get_status() when opcache.restrict_api is used. But you can still use opcache_invalidate(), so it provides enough confirmation that it is loaded, without conflicting with opcache.restrict_api

memtkmcc’s picture

memtkmcc commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new opcache-ok.jpg184.49 KB

#23 works great and applies cleanly -- see the screenshot. Thanks!

opcache ok

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.1.x-dev » 8.2.x-dev
Issue tags: +beta deadline

This issue includes a small API addition, so raising visibility for the beta deadline.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x, thanks!

  • catch committed c32b474 on 8.2.x 
    Issue #2639878 by RobLoach, omega8cc, memtkmcc: Use less demanding check...
omega8cc’s picture

omega8cc commented

Thank you!

  • catch committed c32b474 on 8.3.x 
    Issue #2639878 by RobLoach, omega8cc, memtkmcc: Use less demanding check...

  • catch committed c32b474 on 8.3.x 
    Issue #2639878 by RobLoach, omega8cc, memtkmcc: Use less demanding check...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.