Fix APC tests and add APCu support

[mentalstring]

The APC support has been broken for a while now (see here and here) but since sfAPCCacheTest is often skipped, it's been overlooked.

The issue is that APC has been dropped in more recent versions of PHP and the user cache part of it is now provided by APCu. However, the functions have been changed (eg: apc_get() to apcu_get(), and while there was a compatibility layer for a while it doesn't seem to be maintained anymore. These days one needs to use APC or APCu, often as a requirement by the PHP being used. In other words, sfAPCCache, as is, no longer works in recent versions of PHP where only APCu is available.

This PR fixes the issues with APC inconsistencies when using with negative TTLs, and also adds a new sfAPCuCache that is a drop-in replacement for sfAPCCache. It's pretty minor changes from sfAPCCache, and while it is duplicated code, it's likely one of the cleanest ways to address this without overcomplicating things. sfAPCCacheTest loads the right one according to which extension is present (apc vs apcu).

Lastly, not sure if we should now drop the line bellow allow travis.yml to run the tests with APC (&APCu) again.

symfony1/.travis.yml

Line 36 in 6f521e9

- sh -c 'if [ $(php -r "echo PHP_MINOR_VERSION;") -le 4 ] && [ $(php -r "echo PHP_MAJOR_VERSION;") -le 5 ]; then echo "extension = apc.so" >> ~/.phpenv/versions/$(phpenv version-name)/etc/php.ini; fi;'

[alquerci]

Not sure if we need to keep the support for negative lifetime.

Also, 0 means for APC forever.

What do your think?

[mentalstring]

At least from a sfAPCCache viewpoint, I don't see a particular use for negative TTLs (which from the APC side, is not a documented behavior as a way to invalidate a key).

Looking through the docs, I didn't find any reference about using negative TTL that could lead developers to trust that behavior.

Hence, I think negative TTLs were used here only for convenience of testing. This particular test is to make sure keys expire — not that negative TTLs have that effect. One could argue that the use of sleep() is actually a better way to test that keys do expire over time using a regular (ie: positive) TTL. Negative TTLs sure are convenient for testing, and allow faster test runs, but positive TTLs and sleep() actually makes sure keys expire with the most common use case (positive TTLs).

In any case, mind we are not changing sfAPCCache behavior here, so there's no BC break. The only change is the way we test key expiration.

[alquerci]

LGTM

This patch is no-op until developers change the configuration.

• What about the factory configuration to update by developers?
• What about doing an iteration on the enabled switch as it was added to fix the issue (perhaps) that this PR is fixing (e.g. 5a0adce) ? cc @jeromemacias is it?
• Who is responsible to choose the good APC implementation, the framework or developers?

[alquerci]

Tests failed at least on PHP 7.0

                                                                       
  Error: Class name must be a valid object or a string                 
  (in test/unit/cache/sfAPCCacheTest.php on line 26)                   
                                                                       

To reproduce, I invite you to use #262 with the following command:

$ test/bin/test php70 highest test/unit/cache/sfAPCCacheTest.php

[mentalstring]

Error: Class name must be a valid object or a string
(in test/unit/cache/sfAPCCacheTest.php on line 26)

Should be good now. @alquerci You might want to set all APCU_VERSION on docker-compose.yml now to get this to run.

• What about the factory configuration to update by developers?
• Who is responsible to choose the good APC implementation, the framework or developers?

The choice between apc or apcu always needs to be a deliberate choice by the developer (according to what's available on the environment), just like they could choose to use sfMemcacheCache instead.

• What about doing an iteration on the enabled switch as it was added to fix the issue (perhaps) that this PR is fixing (e.g. 5a0adce) ? cc @jeromemacias is it?

I think it would mostly just add complexity with little or no benefit? Personally I find the current approach to be the cleanest, as one decides to use one or the other (eg: on factories) without having to introduce a new setting.

Then again, it's not clear to me why that enable switch was added to sfAPCCache as there's already a sfNoCache driver for that purpose.

[alquerci]

Test failed, can you reproduce?

test/bin/test php72 highest

// .env
APC_ENABLE_CLI=on

                                                                       
  unit/storage/sfCacheSessionStorageTest.php                           
                                                                       

#   at unit/storage/sfCacheSessionStorageTest.php line 56
>   ->regenerate() regenerated the session with a different session id
      ''
          ne
      ''
#   at unit/storage/sfCacheSessionStorageTest.php line 60
>   ->regenerate() regenerated the session with a different session id
      ''
          ne
      ''

[mentalstring]

It seems sfCacheSessionStorageTest.php uses sfAPCCache... and since the test has been skipped without APC, we are only now finding that the test has been broken since PHP 7.2 at least.

The first is issue that this unit test should probably not depend on APC? Using sfFileCache here would be a more conservative option since APC is not always available, which is what caused this not being caught earlier.

As for what is failing, from my understanding, the issue is that PHP session functions got more strict from PHP 7.2 onwards. Previously, even if the response "headers" (or output on cli) would have already been sent out, the session functions would still work (or just issue a warning), but now they behave differently. In sfCacheSessionStorageTest.php, a simple echo from the lime test framework` is enough for PHP to consider it is too late for some session functions to work.

I tested a workaround for this which was to wrap part of sfCacheSessionStorageTest.php with ob_start(). While perhaps a bit unorthodox, it does the trick just fine: no output is generated until we want it to, making the session functions work normally in this context. If something fails, PHP outputs the buffer anyway before exiting. We could condition the use of the output buffer only for PHP7.2+. Another option is to accept that this test is broken above PHP 7.2 and skip it only for those versions.

I think the ob_start() could work nicely here and can submit the PR for that approach if we agree it's a good compromise towards getting all tests to work when APC enable (although I would still prefer sfFileCache was used here).

[alquerci]

Nice investigation.

Since PHP 7.2 this warning is triggered

Warning: session_id(): Cannot change session id when headers already sent in /app/lib/storage/sfCacheSessionStorage.class.php on line 262

I would still prefer sfFileCache was used here

It seems a good track, but apply this modification does not fix the issue.

[mentalstring]

It seems a good track, but apply this modification does not fix the issue.

Yes, replacing the use of sfAPCCache doesn't fix the test — it's just a potential (additional) improvement to reduce dependencies on APC which is not being tested here.

The proposed solution is to use output buffering with ob_start() — I tried and it works. What do you think of the idea though?

[alquerci]

The proposed solution is to use output buffering with ob_start() — I tried and it works. What do you think of the idea though?

Yeah, let's go for it.

[alquerci]

it's just a potential (additional) improvement to reduce dependencies on APC which is not being tested here.

Indeed, good point, using this idea, I suggest the sfSQLiteCache is simpler and faster to use as it is on memory.
Using sfFileCache is kind of complex as it need to create and clear the directory.

This PR will enable APC cache for all versions on test, but tricky as sfAPCCache on work on PHP version < 7.

[mentalstring]

The proposed solution is to use output buffering with ob_start() — I tried and it works. What do you think of the idea though?

Just noticed that this is exactly what sfSessionStorageTest.php already does (note the difference with sfCacheSessionStorageTest.php), so it doesn't seem so unorthodox after all. It doesn't check for the PHP version nor tries to buffer only part of the test, so I'll follow the same pattern to keep it simple.

Indeed, good point, using this idea, I suggest the sfSQLiteCache is simpler and faster to use as it is on memory. Using sfFileCache is kind of complex as it need to create and clear the directory.

This would replace the dependency on APC by a dependency on sqlite.sfFileCacheTest.php is already part of the test suite and it runs fine by using tempnam().

[jeromemacias]

* What about doing an iteration on the enabled switch as it was added to fix the issue (perhaps) that this PR is fixing (e.g. [5a0adce](https://github.com/FriendsOfSymfony1/symfony1/commit/5a0adceb79d44b76f3ad7d39d041133fafe065f7)) ? cc @jeromemacias is it?

Sorry, I don't remember exactly why I did this change, but I think it's hard to be sure at 100% that the environement have APC installed and well configured so in some cases we cannot tell to use sfNoChache adapter. I's easier for me to say "Use APC if available".

By the way, this PR looks good to me.

[alquerci]

Sorry, I don't remember exactly why I did this change, but I think it's hard to be sure at 100% that the environement have APC installed and well configured so in some cases we cannot tell to use sfNoChache adapter. I's easier for me to say "Use APC if available".

Alright, it makes sense.

The feature is it to have a dynamic service configuration depending on the system configuration?

---

For comparison with latest Symfony cache version.

https://github.com/symfony/symfony/blob/4274a1f/src/Symfony/Component/Cache/Adapter/ApcuAdapter.php#L30

[alquerci]

On this PR there are 2 commits that turn it to un-mergeable state.

• Add support for SameSite cookie session setting => Add support for SameSite session cookies #215
• PHP8.0 & 8.1 support => PHP 8.0 & 8.1 #266

[mentalstring]

Closing this in favor of #267 to get it from a different branch.