Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate config to PHP #810

Merged
merged 3 commits into from
Dec 19, 2023
Merged

Migrate config to PHP #810

merged 3 commits into from
Dec 19, 2023

Conversation

franmomu
Copy link
Contributor

@franmomu franmomu commented Dec 13, 2023

Closes #807

I've made some changes that we need to see if they are fine.

  • I've split the file because if was easier to check.
  • Removed unused services (I haven't found any usage and seem old):
    • doctrine_mongodb.odm.cache as alias of doctrine_mongodb.odm.cache.array
    • doctrine_mongodb.odm.cache.array
  • Removed some parameters ending in .class and use the class directly in the service as second argument.
  • Change these alias:
<service id="%doctrine_mongodb.odm.document_manager.class%" alias="doctrine_mongodb.odm.document_manager" public="false" />
<service id="%doctrine_mongodb.odm.class%" alias="doctrine_mongodb" public="false" />

I'm not sure if that can be done in PHP and I've changed them to (I guess we can change them in the extension resolving the parameter, but not sure if that's necessary):

->alias(DocumentManager::class, 'doctrine_mongodb.odm.document_manager')
->alias(ManagerRegistry::class, 'doctrine_mongodb')

@franmomu franmomu added the Task label Dec 13, 2023
@franmomu franmomu added this to the 5.0.0 milestone Dec 13, 2023
@franmomu franmomu force-pushed the config_php branch 2 times, most recently from 49fd8d4 to 0bf8da2 Compare December 13, 2023 16:22
@GromNaN GromNaN self-requested a review December 13, 2023 16:26
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting the file is a very good idea.

We should get rid of all the .class parameters (see symfony/symfony#11881)

->set('doctrine_mongodb.odm.cache.xcache.class', 'Doctrine\Common\Cache\XcacheCache')
->set('doctrine_mongodb.odm.connection.class', Client::class)
->set('doctrine_mongodb.odm.configuration.class', Configuration::class)
->set('doctrine_mongodb.odm.document_manager.class', DocumentManager::class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter should be deprecated. If someone really wants to change the class for this service, they should update its definition in a compiler pass.

I propose to deprecate updating this parameters in 4.7

Resources/config/mongodb.php Outdated Show resolved Hide resolved
Resources/config/mongodb.php Outdated Show resolved Hide resolved
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@@ -17,8 +17,10 @@
use Doctrine\Common\Cache\RedisCache;
use Doctrine\Common\DataFixtures\Loader as DataFixturesLoader;
use Doctrine\Common\EventSubscriber;
use Doctrine\ODM\MongoDB\Configuration as MongoDBConfiguration;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm nitpicking, but it's all mongodb here. We can specify ODM.

Suggested change
use Doctrine\ODM\MongoDB\Configuration as MongoDBConfiguration;
use Doctrine\ODM\MongoDB\Configuration as ODMConfiguration;

Comment on lines +24 to +34
->set('doctrine_mongodb.odm.cache.array.class', 'Doctrine\Common\Cache\ArrayCache')
->set('doctrine_mongodb.odm.cache.apc.class', 'Doctrine\Common\Cache\ApcCache')
->set('doctrine_mongodb.odm.cache.apcu.class', 'Doctrine\Common\Cache\ApcuCache')
->set('doctrine_mongodb.odm.cache.memcache.class', 'Doctrine\Common\Cache\MemcacheCache')
->set('doctrine_mongodb.odm.cache.memcache_host', 'localhost')
->set('doctrine_mongodb.odm.cache.memcache_port', 11211)
->set('doctrine_mongodb.odm.cache.memcache_instance.class', 'Memcache')
->set('doctrine_mongodb.odm.cache.xcache.class', 'Doctrine\Common\Cache\XcacheCache')
->set('doctrine_mongodb.odm.metadata.driver_chain.class', MappingDriverChain::class)
->set('doctrine_mongodb.odm.metadata.attribute.class', AttributeDriver::class)
->set('doctrine_mongodb.odm.metadata.xml.class', XmlDriver::class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to #811, the parameters should be deprecated here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@GromNaN GromNaN Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we stop using them, we have to keep and deprecate them in 2.x, as they could be used in applications.

Copy link
Contributor Author

@franmomu franmomu Dec 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that since these parameters seem to be used for internal configuration, maybe Symfony can try to fetch them first as build parameters (prefixing the parameter with a ".") and fallback to "normal", I'll see if I have time tomorrow to open a PR there for that.

Update: Symfony always calls an abstract method that is implemented in our side to get the parameter name, so the build parameters can be implemented in our side I think.

Update2: We can do it for metadata parameters, but for cache parameters I think need to be done in the AbstractDoctrineExtension, I'll take a look tomorrow if I find some time.

@GromNaN GromNaN merged commit 0e5e5de into doctrine:5.0.x Dec 19, 2023
12 checks passed
@GromNaN
Copy link
Member

GromNaN commented Dec 19, 2023

Thank you @franmomu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants