Check whether opcache is restricted before attempting to invalidate

[KorvinSzanto]

This resolves issue #350

When using the FileSystem driver on a system where opcache.restrict_api is defined in php.ini, stash throws a fatal error on save. With this change, we check that ini setting before attempting to clear cache.

[KorvinSzanto]

I couldn't think of a way to make a test work for this. I think we would need to just restrict the opcache api by default in the test environments

[coveralls]

Coverage decreased (-0.3%) to 95.256% when pulling c06f4aa on KorvinSzanto:hotfix/opcache-invalidate into 69313c9 on tedious:master.

[alexbowers]

Could we not utilise ini_set?

[alexbowers]

Can you produce a test where it is failing with this based on the ini_get method, and we can go from there?

Thanks.

[KorvinSzanto]

@alexbowers I mention above that I cannot get a test working since this would require setting the ini value before running tests. We'd need to have a specific travis runtime for that if we want to test it.

As far as ini_set, what would you set? Obviously PHP isn't going to let you remove a restriction yourself during runtime, is there something else we can set?

[alexbowers]

@KorvinSzanto For now, please can you just provide a code example that fails for you, without requiring ini_set (based on the actual .ini file).

And please can you show me what the ini value is for all the opcache settings?

Just so I can replicate and try to figure out a way to test it on my end too.
Thanks

[KorvinSzanto]

@alexbowers set opcache.restrict_api to a directory other than where your stash code is located. A good example is to restrict it to a directory that doesn't exist:

opcache.restrict_api=/some/diectory/that/doesnt/exist

Then run the tests. You'll see they fail with a bunch of errors without this pull request and complete happily with it.

[KorvinSzanto]

Here's an example output without this pull request:

$ ./vendor/bin/phpunit
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

.......
..................SSSSSSS...............................  63 / 236 ( 26%)
ESS.ESSSSS.....ESS.ESSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 126 / 236 ( 53%)
SSSSSSSSSS..................................................... 189 / 236 ( 80%)
............................................

Time: 1.41 seconds, Memory: 24.00MB

There were 4 errors:

1) Stash\Test\Driver\FileSystemSerializerTest::testOptionKeyHashFunctionDirs
Zend OPcache API is restricted by "restrict_api" configuration directive

/git/stash/src/Stash/Driver/FileSystem.php:240
/git/stash/src/Stash/Item.php:446
/git/stash/src/Stash/Item.php:406
/git/stash/tests/Stash/Test/Driver/FileSystemTest.php:107

2) Stash\Test\Driver\FileSystemSerializerTest::testStoreData
Zend OPcache API is restricted by "restrict_api" configuration directive

/git/stash/src/Stash/Driver/FileSystem.php:240
/git/stash/tests/Stash/Test/Driver/AbstractDriverTest.php:109

3) Stash\Test\Driver\FileSystemTest::testOptionKeyHashFunctionDirs
Zend OPcache API is restricted by "restrict_api" configuration directive

/git/stash/src/Stash/Driver/FileSystem.php:240
/git/stash/src/Stash/Item.php:446
/git/stash/src/Stash/Item.php:406
/git/stash/tests/Stash/Test/Driver/FileSystemTest.php:107

4) Stash\Test\Driver\FileSystemTest::testStoreData
Zend OPcache API is restricted by "restrict_api" configuration directive

/git/stash/src/Stash/Driver/FileSystem.php:240
/git/stash/tests/Stash/Test/Driver/AbstractDriverTest.php:109

FAILURES!
Tests: 190, Assertions: 2101, Errors: 4, Skipped: 69.

[alexbowers]

Great. I am now able to replicate. Thank you.

[alexbowers]

Okay, I can't find a way to test it, but one thing that we can do I is improve the travis test matrix to have the opcache settings setup by default for some builds to prove that it works with and without opcache.

[KorvinSzanto]

@alexbowers That's what I suggested in my original reply:

I couldn't think of a way to make a test work for this. I think we would need to just restrict the opcache api by default in the test environments

However, I don't think testing this thoroughly is as pressing as implementing a fix.

[alexbowers]

Without a test covering it, I'll have to delegate this to @tedivm since he understands the system far better than I.

[alexbowers]

I have restarted the build, because it appeared to fail based on env. issues.

[tedivm]

So the issue is, if I understand it, that when people have the opcache enabled but have restricted the api are getting errors?

If this request is pulled it means the data will not get invalidated when the files change. That means that stale data would be returned, which would cause some serious issues that are going to be difficult for people to pinpoint.

I think throwing a more specific error about this issue and pushing users on systems with this limitation to the SQLite bases filesystem driver may be a better alternative.

[alexbowers]

So perhaps the fix is to throw a descriptive exception?

Also, @tedivm Are you able to figure out why these tests are failing currently?

[tedivm]

It looks like travis has removed sudo ability and it's required for the hhvm test. It's probably fixable if someone has the time.

[KorvinSzanto]

Was this resolved elsewhere?

[tedivm]

It was not, but the solution here would result in data corruption. I would happily accept a PR that throws an error if opcache is enabled but the API is restricted, like @alexbowers mentioned.

I've also updated the test suite and set it up with Github Actions, so we're in a much better place to pull PRs in now.