search

doctrine / DoctrineORMModule

Doctrine ORM Module for Laminas
https://www.doctrine-project.org/projects/doctrine-orm-module.html
MIT License
437 stars 229 forks source link

Add support for DoctrineModule v6. #734

Closed demiankatz closed 1 year ago

demiankatz commented 1 year ago

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

demiankatz commented 1 year ago

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 commented 1 year ago

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 https://github.com/doctrine/DoctrineModule/pull/805 incorporated.

greg0ire commented 1 year ago

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

TomHAnderson commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Okay, I've made the Psalm changes in https://github.com/doctrine/DoctrineORMModule/pull/734/commits/6880b7de5a1ab61d4a263d2f50193f0fb16daa48 (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 commented 1 year ago

Aha! It looks like CI is copying in a test configuration from a different place. Let's see if https://github.com/doctrine/DoctrineORMModule/pull/734/commits/82fd8c0f6f8f18311f71f0c6525dc53bcb077984 improves things.

demiankatz commented 1 year ago

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 commented 1 year ago

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

demiankatz commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/doctrine/DoctrineModule/pull/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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/doctrine/DoctrineModule/pull/806 and #738, we can get all green checkmarks here.

TomHAnderson commented 1 year ago

https://github.com/doctrine/DoctrineModule/pull/806 and https://github.com/doctrine/DoctrineORMModule/pull/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

TomHAnderson commented 1 year ago

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

demiankatz commented 1 year ago

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.