Refactor the data model, Doctrine, and validation structures #247
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
andEntity
directory, bothDoctrineMongoDBBundle
andDoctrineBundle
will try to autowire mappings for the ODM and ORM respectively. Then add in this bundle'sDoctrineMappingsCompilerPass
, and the end result is an over-configured mapping layer. See this snippet from a compiled container in production now as an example:With these changes, the entity is configured a single time:
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 theRefreshTokenInterface
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 newAbstractRefreshToken
class has all of theNotBlank
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 theAttachRefreshTokenOnSuccessListener
class to trigger the validator, but that's for another PR.Other minor changes include:
DateTime
typehints toDateTimeInterface
, there isn't much reason this can't acceptDateTimeImmutable
as well