#[\AllowDynamicProperties] + fix other issues related to PHP 8.2

[maksimovic]

I became aware of #145 way too late. Here's the PR which is fixing tests / classes by allowing dynamic properties and adjusting several other instances of non-compatible code (${var}, setting a dynamic property to a DOMDocument object, etc).

Adding actual properties seems to be quite daunting work, but if the decision is not to have temporary solution then okay - it will just take way more time (until PHP 9, which, to my knowledge, has no date set).

Even if this PR is accepted, it's by no means a proof that zf1s is ready for PHP 8.2

[glensc]

I don't think anyone has a wish or capacity to review 1 commit in 1509 files. especially since there's no description of the changes, why those files, etc. was it generated somehow? how?

and just blindly trusting your work and merging as is also not reasonable.

can the changes be split into smaller commits, separate pr's? probably should be discussed with maintainers first what is their desire.

what I've seen from zf1-future, is that @hungtrinh has done huge work there to not require the attribute. perhaps that work should be carried here instead?

[maksimovic]

1. I've appended the AllowDynamicProperties to all Zend_ classes in /tests via regex replace, because many of them spawn properties out of thin air, and to some others where it's been needed.
2. After the test files themselves were fixed, from there I worked manually to fix the actual problems found in packages, and fixed whatever issue they've had (based on the output of the failing tests).

The entire work is very simple since the changes are minimal, although the amount of files definitely is huge indeed. By the way, I've literally reviewed every single file myself before making the PR.

You may not like the way of doing this (via attribute), but it's carrying the lowest risk.

You can also review this PR in one go, or review the same amount of work but split into tens for PR's; the amount of actual work will be either the same, or actually more - due to the number of PR's that will really do the same thing overall.

...unless using attributes is a no-go, which makes this PR pointless anyway.

I'm not going to split this very PR into multiple ones, it's what it is - take it or leave it. What I can do is to start crunching smaller PR's per component or something similar, and then make the changes without attributes. It will take a lot of time if I'm going to do that myself, though. I picked this over zf1-future because it offers components as individual packages. The huge project I'm involved with would benefit from ditching the dependency on the entire framework in favor of cherry-picking only the components it requires (currently migrating it to PHP 8.1 from 7.2, but I'd like to have support for 8.2 as well).

[falkenhawk]

@maksimovic thank you for your efforts. That must have costed you quite a lot of your free time to set this up. As you noticed #145 was a similar approach but to be more future-proof it'd be better to add all missing properties to classes which throw notices on php 8.2. As @glensc mentioned @hungtrinh had done it already for zf1-future, and that could be ported over here, only if someone with time at hand would be willing to do that 😅

[maksimovic]

Also, if you collapse the /tests directory at the diff page, the number of files to be inspected is quite bearable. You can trust the edits in /tests I guess - they're doing their work :)

[maksimovic]

@falkenhawk sorry missed your comment. The amount of time invested is maybe 2 hours max, but those were 2 eyebleeding hours definitely.

The decision that needs to be made is whether to use attributes and prolong the more risky/lengthy work until PHP 9 is in sight, or to make the actual changes and create those damn properties where they're supposed to be :)

If the attributes are good to go, then I see no reason to dismiss this PR as a base for more work on compatibility with PHP 8.2. I haven't even tried to let phan/phpcompatibility run against the codebase yet.

However, if the attributes are not the way to go at all, then this PR has very little value. The changes not related to dynamic properties resemble a very, very tiny fraction of the PR.

[maksimovic]

Okay, I see I didn't cover memcached-related stuff - they were most likely skipped in my environment because there was no extension/memcached set up.

[maksimovic]

Okay, this one is weird. Memcached tests here are all throwing:

Creation of dynamic property Memcache::$connection is deprecated

However, after setting up memcached extension & server, when I run the same test I get:

CI=1 vendor/bin/phpunit tests/Zend/Cache/MemcachedBackendTest.php --verbose
PHPUnit 3.7.38 by Sebastian Bergmann.

Configuration read from /Users/oliver/www/zf1s/phpunit.xml.dist

.......................................S.

Time: 41.61 seconds, Memory: 4.00Mb

There was 1 skipped test:

1) Zend_Cache_MemcachedBackendTest::testCleanModeAll
Test fails on CI - memcache->flush() returns false.

/Users/oliver/www/zf1s/tests/Zend/Cache/CommonBackendTest.php:238
/Users/oliver/www/zf1s/vendor/zf1s/phpunit/composer/bin/phpunit:63
OK, but incomplete or skipped tests!
Tests: 41, Assertions: 36, Skipped: 1.

Also, Zend_Memory_MemoryTest::testFactoryCacheBackendStandards with data set #3 ('Memcached') is passing.

Will try digging deeper...

[maksimovic]

Figured it out. Memcache extension's version is too old:

memcache support => enabled
Version => 4.0.5.2

There are 8.0 and 8.2 as it can be seen here: https://pecl.php.net/package/memcache

So, the 4.0.5.2 version of Memcache has a dynamic property and that's what's breaking Memcache tests. There's no way to fix that "from the outside". Looks like PHP setup via shivammathur/setup-php@v2 should be updated, but at the moment I don't know whether the memcache version can be forced or the action itself needs a fix for PHP 8.2.

I've set up my local environment via brew and the memcache which gets installed is v8.2; that's why the tests are passing on my machine.

[maksimovic]

I've managed to install memcache v8.2 with PHP 8.2, but then it was somehow trying to load 2 of its .ini files, which of course started throwing the lovely PHP Warning: Module "memcache" is already loaded in Unknown on line 0 everywhere.

So, my GH Actions skills are ~non-existant~ pretty bad and my solution is probably a bit... barbaric, but I'm open to suggesstions for improving it. The tl;dr is:

• Check the PHP version from the matrix, and if it's 8.2 then request memcache-8.2 to be installed
• At the next step, if it's PHP 8.2 and if there is an offending memcache.ini trying to load the extension again, then obliterate it

Workflow file changes: https://github.com/zf1s/zf1/pull/169/files#diff-1db27d93186e46d3b441ece35801b244db8ee144ff1405ca27a163bfe878957f

I've also left a bit of a debug to print which version of memcache is loaded. I can remove it, but you can see its output in other jobs and verify that the default version installed is the pretty old 4.0.5.2 one.

All tests are now good (well, apart from the incomplete/skipped ones), you can see them in action here (a PR in my fork where I've been banging my head around and finally figured it out).

[falkenhawk]

@maksimovic I plan to eventually extract your changes from this PR, (thank you so much for solving issues with memcache!!) or if you have time at hand maybe you'd like to set up a branch, based on php82-missing-props branch (#170, but it's not finished yet) with all changes other than #[\AllowDynamicProperties]?

[maksimovic]

@falkenhawk I've started to do what you're doing now, but I got entangled into a non-trivial change that took so much time to figure out that in the end I gave it up eventually, and it had a big impact on my motivation to continue.

I'm quite busy with other work these days, so can't promise anything right now. I may do some work and open a PR against your branch to help you out, but most likely not this week.

The issue with memcache was interesting one to solve :)