doctrine / mongodb-odm

The Official PHP MongoDB ORM/ODM
https://www.doctrine-project.org/projects/doctrine-mongodb-odm/en/latest/
MIT License
1.09k stars 504 forks source link

HydratorFactory optimization for possible DI #2421

Closed Eugentis closed 2 years ago

Eugentis commented 2 years ago

Feature Request

Q A
New Feature yes
BC Break yes

Summary

Hi! While I am working with this lib I've detected that you have some strange constructor for HydratorFactory

My DI hits the problem with binding at this factory constructor $hydratorDir, $hydratorNs, $autoGenerate. I understand that it is problem of my DI (it is raw tool right now). But as I can see HydratorFactory works with these 3 params from Doctrine\ODM\MongoDB\Configuration. I suppose it will be better to use some more optimized constructor like:

public function __construct(DocumentManager $dm, EventManager $evm, Configuration $config)
    {
        if (!$config->getHydratorDir()) {
            throw HydratorException::hydratorDirectoryRequired();
        }

        if (!$config->getHydratorNamespace()) {
            throw HydratorException::hydratorNamespaceRequired();
        }

        $this->dm                = $dm;
        $this->evm               = $evm;
        $this->hydratorDir       = $config->getHydratorDir();
        $this->hydratorNamespace = $config->getHydratorNamespace();
        $this->autoGenerate      = $config->getAutoGenerateHydratorClasses();
    }

Or you can use it as dependency Configuration in this class In real this dependency exists but you hide it right now

malarzm commented 2 years ago

It's good to be explicit :) Now anybody instantiating HydratorFactory knows exactly what is needed instead of wondering what should be set in the Configuration object. Anyway, the Configuration object could have been passed, or even not passed at all and relied on DocumentManager's getConfiguration (although this one's fishy as DocumentManager is not yet build when we're HydratorFactory is created).

Anyway, we're not going to change the ctor's signature as it is a BC break (as you noted) for no tangible gain.