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

Refactor the data model, Doctrine, and validation structures #247

Merged
merged 1 commit into from
Jun 29, 2021

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Jun 11, 2021

This aims to refactor the model classes, the Doctrine integration, and the validation integration, to make things a bit easier for maintenance and end-user consumption.

First, the Doctrine integration. Because the bundle ships a Document and Entity directory, both DoctrineMongoDBBundle and DoctrineBundle will try to autowire mappings for the ODM and ORM respectively. Then add in this bundle's DoctrineMappingsCompilerPass, and the end result is an over-configured mapping layer. See this snippet from a compiled container in production now as an example:

    protected function getDoctrine_Orm_DefaultEntityManagerService($lazyLoad = true)
    {
        // snipped

        $a = new \Doctrine\ORM\Configuration();

        $b = ($this->privates['doctrine.system_cache_pool'] ?? $this->getDoctrine_SystemCachePoolService());
        $c = new \Doctrine\Persistence\Mapping\Driver\MappingDriverChain();

        $f = new \Doctrine\ORM\Mapping\Driver\AnnotationDriver(($this->privates['annotations.cached_reader'] ?? $this->getAnnotations_CachedReaderService()), [0 => (\dirname(__DIR__, 4).'/vendor/gesdinet/jwt-refresh-token-bundle/Entity')]);
        $g = new \Doctrine\ORM\Mapping\Driver\XmlDriver(new \Doctrine\Persistence\Mapping\Driver\SymfonyFileLocator([(\dirname(__DIR__, 4).'/vendor/gesdinet/jwt-refresh-token-bundle/Resources/config/orm/doctrine-orm') => 'Gesdinet\\JWTRefreshTokenBundle\\Entity', (\dirname(__DIR__, 4).'/vendor/gesdinet/jwt-refresh-token-bundle/Resources/config/orm/doctrine-entity') => 'Gesdinet\\JWTRefreshTokenBundle\\Entity'], '.orm.xml'));

        $c->addDriver($f, 'Gesdinet\\JWTRefreshTokenBundle\\Entity');
        $c->addDriver($g, 'Gesdinet\\JWTRefreshTokenBundle\\Entity');
        $c->addDriver($g, 'Gesdinet\\JWTRefreshTokenBundle\\Entity');

        $a->setEntityNamespaces(['GesdinetJWTRefreshTokenBundle' => 'Gesdinet\\JWTRefreshTokenBundle\\Entity']);

        // snipped
    }

With these changes, the entity is configured a single time:

    protected function getDoctrine_Orm_DefaultEntityManagerService($lazyLoad = true)
    {
        // snipped

        $a = new \Doctrine\ORM\Configuration();

        $b = new \Doctrine\Persistence\Mapping\Driver\MappingDriverChain();

        $d = new \Doctrine\ORM\Mapping\Driver\SimplifiedXmlDriver([(\dirname(__DIR__, 4).'/vendor/gesdinet/jwt-refresh-token-bundle/Resources/config/doctrine') => 'Gesdinet\\JWTRefreshTokenBundle\\Entity']);
        $d->setGlobalBasename('mapping');

        $b->addDriver($d, 'Gesdinet\\JWTRefreshTokenBundle\\Entity');

        $a->setEntityNamespaces(['GesdinetJWTRefreshTokenBundle' => 'Gesdinet\\JWTRefreshTokenBundle\\Entity']);

        // snipped
    }

The gist of it is that the bundle's entity (when using the ORM) is having mapping drivers registered three times; once for (unused) annotations, and twice for the XML mapping. This can be simplified.

By moving the XML configuration to one of the Doctrine bundle's expected locations, those bundles will take care of automatically registering the appropriate mapping, eliminating the need for a custom compiler pass to register the bundle appropriate compiler passes. This does not affect the ability to use a custom document/entity; by extending the (now empty) document or entity classes, you can still add your own custom data/methods to the token and Doctrine process those changes properly. And in the case of DoctrineBundle, if the ORM isn't even enabled (which the current setup doesn't process), the ORM mapping won't even register.

To simplify things massively, a base Gesdinet\JWTRefreshTokenBundle\Model\AbstractRefreshToken class is provided and it is suggested that any custom implementations of the RefreshTokenInterface extend from this class. The provided document and entity classes now extend from this new class and the abstract document and entity classes are deprecated in favor of this new class.

There is a B/C break here. The doctrine_mappings config from #229 now has no value as far as disabling the Doctrine mappings goes. This should be an acceptable trade-off as it now relies on the Doctrine bundle configs to decide whether the model mappings should be loaded in the first place, which also addresses the issue from #228 where if you're using DoctrineBundle with the ORM disabled then the mapping pass would still be registered.

Now, on the validation logic.

There is also a symfony/validator integration for validating the models. Instead of using annotations (which was a hidden dependency), the validation structure is now provided in XML format. The new AbstractRefreshToken class has all of the NotBlank assertions then separate (extended) validations for the document and entity classes enforce the unique rule. In truth, it would be good to refactor these models to require them to be instantiated in a fully correct state instead of needing the AttachRefreshTokenOnSuccessListener class to trigger the validator, but that's for another PR.

Other minor changes include:

  • Documenting nullable values on the models, since the classes aren't instantiated in a way that forces all the properties to be filled in, any getter can return null
  • Widen the DateTime typehints to DateTimeInterface, there isn't much reason this can't accept DateTimeImmutable as well
  • Marking configuration nodes deprecated through Symfony's API

@mbabker
Copy link
Contributor Author

mbabker commented Jun 11, 2021

This is also another case where #245 needs to land first otherwise there are a lot of things that would have to be adjusted for PHP 5.5 support.

@mbabker mbabker force-pushed the refactor-doctrine-integration branch from 9dc999e to a5e4689 Compare June 28, 2021 21:56
@markitosgv markitosgv merged commit aafe5fb into markitosgv:master Jun 29, 2021
@mbabker mbabker deleted the refactor-doctrine-integration branch June 29, 2021 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants