Fix APC tests and add APCu support #267

mentalstring · 2022-08-03T15:56:40Z

This replaces #263.

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. While there was a compatibility layer it doesn't seem to be maintained anymore, so the functions now have different names (eg: apc_get() to apcu_get(). These days, according to the PHP version use, one needs to use either APC or APCu. In other words, sfAPCCache, as is, no longer works in recent versions of PHP where only APCu is available.

This PR fixes some APC/APCu inconsistencies when using negative TTLs in the tests, 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, I think it's one of 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;'

thePanz

LGTM

Maybe @alquerci (as you reviewed the original PR) would give a feedback too?

alquerci

LGTM

✔️ Tests passes with apcu-5.1.21
🔴 But not with apcu-5.1.22,

I come back since long time, so I went to run all tests.

I got on PHP 7.0 and lower.

Parse error: syntax error, unexpected 'const' (T_CONST), expecting variable (T_VARIABLE) in /app/lib/log/sfLogger.class.php on line 30

It is not a blame, I just expose what I see.

For PHP >7.0 all tests passes, good job ✔️ .

mentalstring · 2023-12-10T21:17:34Z

✔️ Tests passes with apcu-5.1.21
🔴 But not with apcu-5.1.22,

I believe the issue is a not quite planned behaviour change on 5.1.22 that even broke the apc.php info page. This has been reverted on 5.1.23 released a month ago — the test passes again with that.

I don't think it's worth putting a complicated workaround for a specific apcu version — maybe we can just skip() if it's apcu-5.1.22?

I got on PHP 7.0 and lower.

Parse error: syntax error, unexpected 'const' (T_CONST), expecting variable (T_VARIABLE) in /app/lib/log/sfLogger.class.php on line 30

I'm having trouble running the tests from master.

$ docker --version
Docker version 24.0.6, build ed223bc
$ docker-compose --version
Docker Compose version v2.23.0-desktop.1

$ test/bin/test php73 highest test/unit/cache/sfAPCCacheTest.php
+ docker-compose build
failed to solve: process "/bin/sh -c set -eux;     apt-get update;   apt-get install -y     curl     autoconf2.13   ;   rm -r /var/lib/apt/lists/*;     curl -sSLfO http://launchpadlibrarian.net/140087283/libbison-dev_2.7.1.dfsg-1_amd64.deb;   curl -sSLfO http://launchpadlibrarian.net/140087282/bison_2.7.1.dfsg-1_amd64.deb;   dpkg -i libbison-dev_2.7.1.dfsg-1_amd64.deb;   dpkg -i bison_2.7.1.dfsg-1_amd64.deb;   rm *.deb;     curl -sSLf \"https://php.net/get/php-$PHP_VERSION.tar.bz2/from/this/mirror\" -o php.tar.bz2;   echo 'c4e1cf6972b2a9c7f2777a18497d83bf713cdbecabb65d3ff62ba441aebb0091  php.tar.bz2' | sha256sum -cw --status;     mkdir -p /usr/src/php;   tar -xf php.tar.bz2 -C /usr/src/php --strip-components=1;   rm php.tar.bz2*;     cd /usr/src/php;   ./buildconf --force;   ./configure --disable-cgi     $(command -v apxs2 > /dev/null 2>&1 && echo '--with-apxs2' || true)     --with-pdo-mysql     --with-zlib     --enable-mbstring   ;   make -j\"$(nproc)\";   make install;     dpkg -r     bison     libbison-dev   ;   apt-get purge -y --auto-remove     autoconf2.13   ;   rm -r /usr/src/php" did not complete successfully: exit code: 100

Any idea, @alquerci?

alquerci · 2023-12-11T08:47:58Z

Hello @mentalstring

I agree, for the APCu version.

For running tests.

Try with patches.

mentalstring · 2023-12-22T21:58:30Z

I've rebased and added the skip for APCu 5.1.22. Also filled APCU_VERSION to 5.1.23 on docker-compose.yml so that the APCu tests can run.

I think it's ready for merging. But if the minimum version becomes PHP 7.0, then I suppose instead of having two classes (sfAPCCache.class.php and sfAPCuCache.class.php), we could just change the original sfAPCCache.class.php to use the new APCu API. /cc @thePanz

I got on PHP 7.0 and lower.
Parse error: syntax error, unexpected 'const' (T_CONST), expecting variable (T_VARIABLE) in /app/lib/log/sfLogger.class.php on line 30
It is not a blame, I just expose what I see.

For PHP >7.0 all tests passes, good job ✔️ .

Still can't reproduce this. I have tested test/unit/cache/sfAPCCacheTest.php with all PHP versions and also ran the whole test suite with PHP 7.0 and didn't have any problem. Can you still reproduce it @alquerci ?

alquerci · 2023-12-23T18:30:21Z

@mentalstring What steps did you done to execute the whole test suite with PHP 7.0?

I suppose

You apply patches

Execute test/bin/test --php-versions php70

.env.dist

mentalstring · 2023-12-24T00:03:35Z

@mentalstring What steps did you done to execute the whole test suite with PHP 7.0?

I suppose

You apply patches

Fix docker images build to handle old linux distributions #270

Restore support of PHP <7.1 #293

Execute test/bin/test --php-versions php70

Yep, pretty much. The test completed with no problem for me.

alquerci · 2023-12-26T17:15:58Z

After removing APC_ENABLE_CLI setting, for me, this PR can be merged.

@mentalstring without this following patch, you will get the unexpected 'const' parse error.

Restore support of PHP <7.1 #293

But this issue is unrelated with this PR, except for the minimum PHP version supported.
As you said, no need to support APC is the minimum PHP version supported is at least 7.0.

mentalstring · 2023-12-26T19:18:33Z

After removing APC_ENABLE_CLI setting, for me, this PR can be merged.

Should be ready now.

thePanz · 2023-12-30T21:17:00Z

Thanks @mentalstring !
Would you please:

use the conventional commit messages (just a nitpick, not so strongly enforcing it tho)
check if the github workflow pipeline should be updated to always enable the apcu extension in the "php setup" action: this would make sure that we run the tests with the extension

wdyt?

mentalstring · 2023-12-31T11:35:17Z

check if the github workflow pipeline should be updated to always enable the apcu extension in the "php setup" action: this would make sure that we run the tests with the extension

Ah, good point. I think I managed to enable APCu on the CI runs, and I checked that it already installs 5.1.23 so that the test doesn't get skipped.

use the conventional commit messages (just a nitpick, not so strongly enforcing it tho)

Happy to, just not sure if I covered everything... Was it just the line length?

thePanz · 2023-12-31T21:50:53Z

Ah, good point. I think I managed to enable APCu on the CI runs, and I checked that it already installs 5.1.23 so that the test doesn't get skipped.

Nice! Thanks!

Happy to, just not sure if I covered everything... Was it just the line length?
Fine as it is now! I was more for a prefix with "Fix" :) all good now!

One of the commit's long-message reads

as an alternative
to APC which longer works with recent versions of PHP.

It should read "..which no longer works..." instead (the no is missing)

- Using negative TTLs to force the immediate expiration of keys, while convenient in tests, doesn't work consistently with APC and is an undocumented feature. Using a low TTL and sleep() is what guarantees that it works for APC. See krakjoe/apcu#184 - The setting apc.use_request_time interferes with key expiration when running on the CLI. Making sure it always has a sensible value for running the tests. See krakjoe/apcu#392

Support for the APCu extension (through sfAPCuCache) as an alternative to APC, which no longer works with recent versions of PHP.

From PHP 7.2 onward, session functions are stricter and may not work if output/headers have already been sent out. Using output buffering prevents this issue.

Replace the use of sfAPCCache with sfFileCache in sfCacheSessionStorageTest so that it doesn't depend on APC being available.

thePanz · 2024-01-03T08:39:27Z

Merged, thanks @mentalstring ! 🎉

mentalstring mentioned this pull request Aug 3, 2022

Fix APC tests and add APCu support #263

Closed

thePanz force-pushed the apc-and-apcu branch from fe28b07 to 19e8daa Compare January 10, 2023 13:01

thePanz approved these changes Jan 10, 2023

View reviewed changes

thePanz force-pushed the apc-and-apcu branch from 19e8daa to 59704bf Compare January 12, 2023 13:15

alquerci approved these changes Apr 26, 2023

View reviewed changes

mentalstring force-pushed the apc-and-apcu branch from b3fbae5 to 59704bf Compare July 2, 2023 17:41

mentalstring force-pushed the apc-and-apcu branch 4 times, most recently from 987bfb6 to b2af1f7 Compare December 22, 2023 21:47

thirsch approved these changes Dec 23, 2023

View reviewed changes

alquerci reviewed Dec 23, 2023

View reviewed changes

.env.dist Outdated Show resolved Hide resolved

mentalstring force-pushed the apc-and-apcu branch from b2af1f7 to 864409c Compare December 26, 2023 19:07

alquerci approved these changes Dec 26, 2023

View reviewed changes

mentalstring force-pushed the apc-and-apcu branch from 864409c to 0c77ff7 Compare December 30, 2023 19:17

thePanz force-pushed the apc-and-apcu branch from 0c77ff7 to 2569329 Compare December 30, 2023 21:32

mentalstring force-pushed the apc-and-apcu branch from 2569329 to 802c1dd Compare December 31, 2023 11:24

mentalstring added 3 commits January 1, 2024 09:49

Add APCu support

234865d

Support for the APCu extension (through sfAPCuCache) as an alternative to APC, which no longer works with recent versions of PHP.

Fix sfCacheSessionStorageTest with PHP>=7.2

30966ee

From PHP 7.2 onward, session functions are stricter and may not work if output/headers have already been sent out. Using output buffering prevents this issue.

Remove test dependency on APC

058149b

Replace the use of sfAPCCache with sfFileCache in sfCacheSessionStorageTest so that it doesn't depend on APC being available.

mentalstring force-pushed the apc-and-apcu branch from 802c1dd to 058149b Compare January 1, 2024 10:08

thePanz merged commit 5837057 into FriendsOfSymfony1:master Jan 3, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix APC tests and add APCu support #267

Fix APC tests and add APCu support #267

mentalstring commented Aug 3, 2022 •

edited

Loading

thePanz left a comment

alquerci left a comment

mentalstring commented Dec 10, 2023

alquerci commented Dec 11, 2023

mentalstring commented Dec 22, 2023

alquerci commented Dec 23, 2023

mentalstring commented Dec 24, 2023

alquerci commented Dec 26, 2023

mentalstring commented Dec 26, 2023

thePanz commented Dec 30, 2023

mentalstring commented Dec 31, 2023

thePanz commented Dec 31, 2023 •

edited

Loading

thePanz commented Jan 3, 2024

Fix APC tests and add APCu support #267

Fix APC tests and add APCu support #267

Conversation

mentalstring commented Aug 3, 2022 • edited Loading

thePanz left a comment

Choose a reason for hiding this comment

alquerci left a comment

Choose a reason for hiding this comment

mentalstring commented Dec 10, 2023

alquerci commented Dec 11, 2023

mentalstring commented Dec 22, 2023

alquerci commented Dec 23, 2023

mentalstring commented Dec 24, 2023

alquerci commented Dec 26, 2023

mentalstring commented Dec 26, 2023

thePanz commented Dec 30, 2023

mentalstring commented Dec 31, 2023

thePanz commented Dec 31, 2023 • edited Loading

thePanz commented Jan 3, 2024

mentalstring commented Aug 3, 2022 •

edited

Loading

thePanz commented Dec 31, 2023 •

edited

Loading