Add support for DoctrineModule v6.

[demiankatz]

This PR is intended to add compatibility with DoctrineModule version 6, since that release brings some dependencies up to date, and those newer versions will be needed to prevent conflicts with other components.

TODO

• Get phpstan passing (note that the current phpstan failures are also occurring for me on the 5.4.x branch, so they are unrelated to the changes here -- merging Fix PHPStan failures. #736 should take care of them).
• Get psalm passing (the failures here are related to upstream issues in DoctrineModule 6.0.0 -- I have opened Fix setOptions typehints. DoctrineModule#805 to address them, and when it is merged, they should go away).
• Get unit tests passing
• Get integration tests passing (currently blocked by Fix cache configuration for better Doctrine compatibility. DoctrineModule#806, and probably some other issues as well)
• Close Upgrade doctrine/DoctrineModule to v6 #735 when this is merged, as this PR resolves that issue.

[demiankatz]

Maybe this should be refactored to a trait so the logic can be shared between tests. I'm happy to do that if others think it's a good idea.

[demiankatz]

I figured out why the tests were failing -- I just had to include the appropriate Laminas cache storage module in the test setup configuration. Everything is passing now!

I believe I've done everything I can do -- now it's up to the maintainers to review and (hopefully) merge my work. :-)

Note that the phpstan issue probably needs attention, but as that is an existing issue, I'm going to hope it can be addressed separately. If my help is needed, please let me know, though unfortunately my time is quite limited these days!

[demiankatz]

Now that the phpstan issue is fixed and merged, I've rebased this on 5.4.x. I think this is ready to go as soon as we have a fixed release of DoctrineModule with doctrine/DoctrineModule#805 incorporated.

[greg0ire]

@demiankatz sorry, but the test suite is bright red now. Sometimes it's PHPUnit, sometimes it's cli.sh that fails.

[TomHAnderson]

The laminas/laminas-cache used to include laminas/laminas-cache-storage-adapter-memory as a requirement. It is now require-dev. However this module expects it in order to run tests. Add

"laminas/laminas-cache-storage-adapter-memory": "^2.0",

to the composer require-dev and the tests pass locally on PHP 7.4.

The other error is on the cli tests

Fatal error: Uncaught Laminas\ServiceManager\Exception\ServiceNotFoundException: Unable to resolve service "filesystem" to a factory; are you certain you provided it during configuration? in /home/runner/work/DoctrineORMModule/DoctrineORMModule/vendor/laminas/laminas-servicemanager/src/ServiceManager.php:586

I'm at a loss for this one. I'll research it next.

[demiankatz]

@greg0ire, I thought the failing build was due to the Psalm errors. I'll fix those momentarily, and then we'll see what else is going on.

@TomHAnderson, you have to include the cache storage adapter's Laminas module into the module manager in order for the configurations to get injected into the service manager. See my changes to tests/config.php in this PR. I only activated the Memory adapter because that was the only one causing tests to fail in my environment, but maybe there are conditional tests that I wasn't running. We might just need to add the File adapter in the same place. I'll look at that too after sorting out the Psalm stuff.

[demiankatz]

Okay, I've made the Psalm changes in 6880b7d (which I'm happy to cherry-pick into a separate PR if that's preferred, but since the fix is tied to changes introduced here, maybe it's simpler to do it all in one place -- let me know what you prefer). Now all the style checks are green.

I see that tests are still failing with the missing Memory adapter error. This is strange, because they pass when I run composer run test in my local environment. I'll have to dig in and see what is different in the context of GitHub Actions!

[demiankatz]

Aha! It looks like CI is copying in a test configuration from a different place. Let's see if 82fd8c0 improves things.

[demiankatz]

Okay, we're getting somewhere -- this is the configuration where we need to add the FileSystem module to solve @TomHAnderson's problem. I've just pushed that up, and I suspect it will get some green checkmarks, at least on the PHP 8.x builds (or reveal the next problem, if nothing else).

However, it looks like the PHP 7.x builds are complaining because they can't find the necessary modules. Presumably this means that composer isn't installing those for some reason (probably because different dependency versions are getting loaded). Is it worth trying to fix that, or should we just drop PHP 7 support since it is at end of life and target this as a new major version (if a major version is required for a PHP version requirement bump)?

[demiankatz]

Oops, case sensitivity error -- it's Filesystem, not FileSystem. Let's try that again!

[demiankatz]

Okay, that's progress -- now it's complaining about a missing or inaccessible directory. I'm not sure why, but at least the right code is loading. I'll keep investigating.

[demiankatz]

Okay, I added a new CI step to create the cache directory, which seems to have helped (not sure if that's the best approach, so I'm open to other ideas, but at least it works!).

Now there's a new problem in the integration tests:

In AbstractAdapter.php line 1492:
                                                                               
  The key 'DoctrineNamespaceCacheKey[]' doesn't match against pattern '/^[a-z  
  0-9_\+\-]*$/Di'                                                              

My guess is that the Laminas cache layer has a more restrictive regex for cache keys than the previous Doctrine cache layer did, but I'm not too familiar with the context or implementation details of any of this. I'll keep looking, but if @greg0ire or @TomHAnderson have any ideas, I'd welcome input on this one!

[demiankatz]

Okay, so the cache key containing brackets is coming from the doctrine-cache module here:

https://github.com/doctrine/cache/blob/2.2.x/lib/Doctrine/Common/Cache/CacheProvider.php#L15

The key exception is coming from here:

https://github.com/laminas/laminas-cache/blob/3.11.x/src/Storage/Adapter/AbstractAdapter.php#L1493

It looks like the key pattern is configurable, so I think we should be able to adjust it in whatever factory is building the Laminas cache adapters... but we may have to do that over in DoctrineModule before we can get tests to pass here. I'll keep digging!

[demiankatz]

Pretty sure we just need to add something here:

https://github.com/doctrine/DoctrineModule/blob/a63fb62b45a142fb86feafbd542ec8492f18b01f/src/ConfigProvider.php#L118

...but it will take me a little bit to figure out how to test it properly. :-)

[demiankatz]

This rabbit hole keeps getting deeper. I figured out how to run the CI process in my test environment, which allowed me to write code to configure the cache key -- see doctrine/DoctrineModule#806. I don't think we'll be able to get further on this PR until that code is merged and released, but it's possible that if we get deeper into this process, we'll discover that more characters need to be allowed in the cache key. Bit of a chicken and egg situation! Thus, it's probably a good idea to try to figure out how to get the whole process passing locally before fully committing to the DoctrineModule solution.

When I locally patch DoctrineModule to allow the cache keys, the next problem I run into is:

PHP Fatal error:  Uncaught Error: Object of class Doctrine\ORM\Query\ParserResult could not be converted to string in /home/dkatz/DoctrineORMModule/vendor/laminas/laminas-cache-storage-adapter-filesystem/src/Filesystem.php:1001
Stack trace:
#0 /home/dkatz/DoctrineORMModule/vendor/laminas/laminas-cache/src/Storage/Adapter/AbstractAdapter.php(664): Laminas\Cache\Storage\Adapter\Filesystem->internalSetItem()
#1 /home/dkatz/DoctrineORMModule/vendor/laminas/laminas-cache-storage-adapter-filesystem/src/Filesystem.php(881): Laminas\Cache\Storage\Adapter\AbstractAdapter->setItem()
#2 /home/dkatz/DoctrineORMModule/vendor/doctrine/doctrine-module/src/Cache/LaminasStorageCache.php(47): Laminas\Cache\Storage\Adapter\Filesystem->setItem()
#3 /home/dkatz/DoctrineORMModule/vendor/doctrine/cache/lib/Doctrine/Common/Cache/CacheProvider.php(115): DoctrineModule\Cache\LaminasStorageCache->doSave()
#4 /home/dkatz/DoctrineORMModule/vendor/doctrine/cache/lib/Doctrine/Common/Cache/Psr6/CacheAdapter.php(235): Doctrine\Common\Cache\CacheProvider->save()
#5 /home/dkatz/DoctrineORMModule/vendor/doctrine/cache/lib/Doctrine/Common/Cache/Psr6/CacheAdapter.php(183): Doctrine\Common\Cache\Psr6\CacheAdapter->commit()
#6 /home/dkatz/DoctrineORMModule/vendor/doctrine/orm/lib/Doctrine/ORM/Query.php(276): Doctrine\Common\Cache\Psr6\CacheAdapter->save()
#7 /home/dkatz/DoctrineORMModule/vendor/doctrine/orm/lib/Doctrine/ORM/Query.php(286): Doctrine\ORM\Query->parse()
#8 /home/dkatz/DoctrineORMModule/vendor/doctrine/orm/lib/Doctrine/ORM/AbstractQuery.php(1212): Doctrine\ORM\Query->_doExecute()
#9 /home/dkatz/DoctrineORMModule/vendor/doctrine/orm/lib/Doctrine/ORM/AbstractQuery.php(1166): Doctrine\ORM\AbstractQuery->executeIgnoreQueryCache()
#10 /home/dkatz/DoctrineORMModule/vendor/doctrine/orm/lib/Doctrine/ORM/Tools/Console/Command/RunDqlCommand.php(119): Doctrine\ORM\AbstractQuery->execute()
#11 /home/dkatz/DoctrineORMModule/vendor/symfony/console/Command/Command.php(326): Doctrine\ORM\Tools\Console\Command\RunDqlCommand->execute()
#12 /home/dkatz/DoctrineORMModule/vendor/symfony/console/Application.php(1063): Symfony\Component\Console\Command\Command->run()
#13 /home/dkatz/DoctrineORMModule/vendor/symfony/console/Application.php(320): Symfony\Component\Console\Application->doRunCommand()
#14 /home/dkatz/DoctrineORMModule/vendor/symfony/console/Application.php(174): Symfony\Component\Console\Application->doRun()
#15 /home/dkatz/DoctrineORMModule/vendor/doctrine/doctrine-module/bin/doctrine-module.php(45): Symfony\Component\Console\Application->run()
#16 /home/dkatz/DoctrineORMModule/vendor/doctrine/doctrine-module/bin/doctrine-module(4): include('...')
#17 /home/dkatz/DoctrineORMModule/vendor/bin/doctrine-module(117): include('...')
#18 {main}
  thrown in /home/dkatz/DoctrineORMModule/vendor/laminas/laminas-cache-storage-adapter-filesystem/src/Filesystem.php on line 1001

Looks like an object is getting passed where a string is expected. I'm nearly out of time for today, so I haven't gone digging further into this yet. Any thoughts, @greg0ire or @TomHAnderson?

[demiankatz]

Is it possible that the Doctrine cache adapter did automatic serialization of objects, but the Laminas adapter does not? I haven't gone digging to test that theory, but it seems like a possible explanation for what I'm seeing.

[demiankatz]

Ha, I've got it!!! My theory was correct; there's a missing Laminas Filesystem configuration to turn on serialization, which creates functionality equivalent to the old Doctrine cache adapter. With this change in place, the integration suite passes on my local system. I think if we can merge doctrine/DoctrineModule#806 and #738, we can get all green checkmarks here.

[TomHAnderson]

doctrine/DoctrineModule#806 and #738 have been merged. I've added branch 6.0.x to this repository and changed this PR to commit into it. Please update composer.json to require DoctrineModule@^6.0.2

[demiankatz]

Thanks, @TomHAnderson, I've rebased this onto 6.0.x and adjusted the composer.json as requested. Fingers crossed that we get a clean build now. Please let me know if you'd like me to do anything else -- I'm happy to squash commits, etc., if that would be helpful to you.

[demiankatz]

Okay, this is progress, but we're not quite there yet. All the "highest" builds are passing, but there are problems with "lowest." I'll look into exactly what this means, but I wonder if the simplest solution may be to raise some other requirements, if this has to do with supporting old things that nobody is using anyway.

[TomHAnderson]

It's a fresh version so adjust as you see fit.

[TomHAnderson]

Squashing commits isn't something I usually do. There are exceptions but I don't see the harm in seeing a whole history.

[demiankatz]

Thanks again, @TomHAnderson. I think I've got it now -- the problem is that DoctrineModule 5 includes the Laminas cache storage adapters as dev dependencies, whereas DoctrineModule 6 includes them as primary dependencies. That means we need to also add them as dev dependencies here so that when we composer install --prefer-lowest they're still available even in combination with the older DoctrineModule.

My commit history got very sloppy here from all my trial and error, so I've done a bit of squashing so it can look cleaner when merged. :-)

It looks like the whole build is passing now!

Please feel free to merge at your convenience -- I'm excited at the prospect of getting a new release of this module so my own project can move forward! Thanks for all of your help moving the various pieces along.