doctrine-extensions
Introduce the Revisionable extension
Ref: #2502
As the issue notes, the loggable extension is incompatible with ORM 4.x due to the removal of the array field type. The issue also has a patch for migrating to JSON, but because of the need for a data migration, it's not a patch that can be easily dropped in. Enter the new revisionable extension.
Mostly the same thing as the loggable extension, except for changing the way the data is collected for the history object's data field. For the ORM, the data was stored as a serialized PHP array, which while it works, is less than optimal:
a:4:{s:5:"title";s:5:"Title";s:9:"publishAt";O:17:"DateTimeImmutable":3:{s:4:"date";s:26:"2024-06-24 23:00:00.000000";s:13:"timezone_type";i:3;s:8:"timezone";s:3:"UTC";}s:11:"author.name";s:8:"John Doe";s:12:"author.email";s:12:"[email protected]";}
The new extension uses the mapping layers of the ORM and ODM to store the data in its database representation, and for the ORM, as a native JSON field:
{
"title": "Title",
"publishAt": "2024-06-24 23:00:00",
"author.name": "John Doe",
"author.email": "[email protected]"
}
Another benefit to introducing a new extension is that the old and new versions can live side-by-side and allows for a data migration so long as your log entry data can be unserialized back into PHP and its info mapped to a database value (https://gist.github.com/mbabker/1879a3e55feac953e23cb2f025654052 was something I quickly hacked into the example app code as a proof of concept, a full-on tool could be built out using the logic here).
There are some other extras baked into this branch which generally help improve things too, including:
- Adding templates for the config arrays in the extension listeners
- Adding new methods to the
WrapperInterfaceimplementations (and@methodannotated on the interface itself to add to 4.0) to handle mapping values to PHP and database formats for each manager
Differences between the two extensions include:
- Removing the
prePersistLogEntry()hook that existed in the loggable listener, the Doctrine event manager can be used here without needing to have an open class - Uses a
Revisionableattribute instead ofLoggableat the class level, and uses aKeepRevisionsattribute instead of theVersionedattribute at the property level; for migrating, you're basically looking at a handful of lines changed no matter the mapping (changing a class import and annotation/attribute when using that mapping, XML goes from<gedmo:loggable/>to<gedmo:revisionable/>and<gedmo:versioned/>to<gedmo:keep-revisions/>) - Fully typed to the extent PHP 7.4 allows
Having no username set in the RevisionableListener doesn't work because ->setUsername() expects a string even though it is nullable in the database
Having no username set in the
RevisionableListenerdoesn't work because->setUsername()expects a string even though it is nullable in the database
Thanks, I've updated everything to account for that.
Codecov Report
:x: Patch coverage is 85.23132% with 83 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 79.14%. Comparing base (e60de54) to head (2372808).
Additional details and impacted files
@@ Coverage Diff @@
## main #2825 +/- ##
==========================================
+ Coverage 78.75% 79.14% +0.39%
==========================================
Files 169 182 +13
Lines 8695 9256 +561
==========================================
+ Hits 6848 7326 +478
- Misses 1847 1930 +83
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
What's the status of this PR? Anything I can do to help/test? Would like to see it merged soon!
What's the status of this PR? Anything I can do to help/test? Would like to see it merged soon!
I don't think I have anything big to add to this PR at this point. It really just needs testing and making sure the idea is solid (especially as part of the rationale for a new extension versus trying to make a version of loggable that is DBAL 4 friendly is allowing both the old and new data to live side-by-side so an app can either keep that old data by either polyfilling the deprecated DBAL array type or migrating it into the new extension).
I started trying to test/use this in an ODM configuration.
Notables so far:
- RevisionableListener is final - I was previously extending LoggableListener, but perhaps will be able to make the necessary changes in a RevisionInterface implementation instead.
- RevisionInterface "sets" has lots of
voidreturns - In LogEntryInterface these weren't hardcoded, perhaps not the end of the world. - RevisionInterface has a lot of string that are no longer
nullable- Again, probably fine for now. - Missing
StofDoctrineExtensionsBundleintegration - Biggest stumpling block so far as event listeners aren't triggering at all. - No documentation/examples - i.e.
doc/revisionable.mdand updateddoc/annotations.md
So at the moment I've not been able to create/trigger any revisions, I guess there is a way to enable this without StofDoctrineExtensionsBundle, but if you were able to create a dev version with support then I'll be able to test further.
EDIT:
Yes, the nullable change's are causing problems.
e.g.
ERROR: InvalidNullableReturnType - src/Document/Revision.php:82:34 - The declared return type 'string' for
App\Document\Revision::getAction is not nullable, but 'null|string' contains null (see https://psalm.dev/144)
public function getAction(): string
RevisionableListener is final - I was previously extending LoggableListener, but perhaps will be able to make the necessary changes in a RevisionInterface implementation instead.
The loggable listener has to be open because of its prePersistLogEntry hook point; with this implementation, I decided to rely on the events that the Doctrine object managers emit and get rid of that, and as a result, I've landed on a final class.
Maybe it's too soon to go with a hard final listener, but I'm not going to take the "no I'm not going to open it up for inheritance" standpoint if there's a legitimate use case that a hard final blocks. Maybe somewhere down the road, something can be figured out for how interfaces can be built for the listeners (as right now the hook points into Doctrine are all Doctrine event listeners, so the interface is more so specifying the required events to subscribe to and less what a public-ish API looks like).
RevisionInterface "sets" has lots of void returns - In LogEntryInterface these weren't hardcoded, perhaps not the end of the world.
The new extension is more strictly typed given it's being written as new code with the PHP 7.4 minimum in mind, compared to all of the other extensions which were written in PHP 5 times; LogEntryInterface already uses @return void annotations so RevisionInterface solidifies this with native return types.
RevisionInterface has a lot of string that are no longer nullable - Again, probably fine for now.
I wanted to make the revision model a little more strict in terms of having valid state, hence the introduction of the static RevisionInterface::createRevision() constructor and the listener using that over new Revision(); the three fields this effects are the action, version, and logged at timestamp (all of which can be initialized in those constructors), the rest of the fields do remain nullable as the model is designed for everything else to support null values (you don't have to have a user for a revision, you don't have to log the data state for a revision, etc.).
Missing
StofDoctrineExtensionsBundleintegration - Biggest stumpling block so far as event listeners aren't triggering at all.
That'd have to be done after this PR landed. Trying to shoot a PR off over there would just result in a PR with constantly failing builds, it's easier to handle that update after this change lands.
For manual config in a Symfony app, you can take the loggable listener service in https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/doc/frameworks/symfony.md#extensions-compatible-with-all-managers and change that to the revisionable listener (it'll hook into the same events so it's just changing the service ID and class name)
No documentation/examples - i.e.
doc/revisionable.mdand updateddoc/annotations.md
It'll get done before this merges. I did call out the needed mapping changes in the PR description, but the gist of it is you switch @Gedmo\Loggable to @Gedmo\Revisionable for annotations and attributes, and if you're using the logEntryClass param to use a custom model, change that to revisionClass. The @Gedmo\Versioned annotation/attribute gets reused, with the goal here being to make the config migration as minimal as possible.
If action is no longer nullable does it make sense set a default then, to avoid getAction() complaining?
e.g.
#[ODM\Field(name: 'action', type: Type::STRING)]
- protected ?string $action = null;
+ protected string $action = self::ACTION_CREATE;
(or whatever, if the syntax is slightly off)
If action is no longer nullable does it make sense set a default then, to avoid
getAction()complaining?
Probably; this has all been iterated over a few times so little pain points like that are bound to have slipped in by accident.
If action is no longer nullable does it make sense set a default then, to avoid
getAction()complaining?Probably; this has all been iterated over a few times so little pain points like that are bound to have slipped in by accident.
This has been updated.
For manual config in a Symfony app, you can take the loggable listener service and change that to the revisionable listener
For reference this is what I'm using for now.
services:
gedmo.mapping.driver.attribute:
class: Gedmo\Mapping\Driver\AttributeReader
gedmo.listener.revisionable:
class: Gedmo\Revisionable\RevisionableListener
tags:
- { name: doctrine_mongodb.odm.event_listener, event: 'onFlush' }
- { name: doctrine_mongodb.odm.event_listener, event: 'loadClassMetadata' }
- { name: doctrine_mongodb.odm.event_listener, event: 'postPersist' }
calls:
- [ setAnnotationReader, [ '@gedmo.mapping.driver.attribute' ] ]
Then to get setUsername() working an additional #[AsEventListener(event: KernelEvents::REQUEST)].
Next problem, there seems to be a circular issue with ODM embedded documents.
- To appear in revision data the field in the embedded document requires versioned attribute.
- But this gives a mapping exception about missing revisionable attribute.
- Adding the revisionable attribute gives another mapping exception that embedded objects can't have a revisionable attribute.
In fairness the same problem exists with loggable, I guess it worked at some point, then got broken.
I would suggest that embedded documents should require versioned attributes against fields, but not revisionable/loggable attributes against the class.
In testing this works. i.e. revision (and log) data gets set for embedded documents.
Looks like this was previsouly enabled for ORM (isEmbeddedClass?), but the one that fixes it for me in ODM is isEmbeddedDocument.
e.g. src/Revisionable/Mapping/Driver/Attribute.php
// Validate configuration
if (!$meta->isMappedSuperclass && $config) {
// Invalid when the versioned config is set and the revisionable flag has not been set
- if (isset($config['versioned']) && !isset($config['revisionable']) {
+ if (
+ isset($config['versioned']) && !isset($config['revisionable']) &&
+ (!isset($meta->isEmbeddedClass) || !$meta->isEmbeddedClass) &&
+ (!isset($meta->isEmbeddedDocument) || !$meta->isEmbeddedDocument)
+ ) {
throw new InvalidMappingException(sprintf("Class '%s' has '%s' annotated fields but is missing the '%s' class annotation.", $meta->getName(), Versioned::class, Revisionable::class));
}
}
src/Loggable/Mapping/Driver/Attribute.php
protected function isClassAnnotationInValid(ClassMetadata $meta, array &$config)
{
- return isset($config['versioned']) && !isset($config['loggable']) && (!isset($meta->isEmbeddedClass) || !$meta->isEmbeddedClass);
+ return isset($config['versioned']) && !isset($config['loggable']) && (!isset($meta->isEmbeddedClass) || !$meta->isEmbeddedClass) && (!isset($meta->isEmbeddedDocument) || !$meta->isEmbeddedDocument);
}
Do you have an example you can share where it's not working? I'm not a ODM user, but I went through the effort to get MongoDB running in my setup for a couple OSS projects so I was able to build out the test fixtures for this new code, which include embedded documents.
One of the differences between the ODM and ORM is how they track embedded objects. The ORM inlines embeds as part of the entity itself, while the ODM tracks them as managed objects in the unit of work. An earlier iteration tried to not use the Revisionable attribute on embedded documents, but because of that difference between the two object managers, the embedded document wasn't being handled in the event listener.
Not sure what you're after exactly, but here's a slightly stripped back example of what I'm testing with, from a Symfony project.
App/Document/Product.php
<?php
namespace App\Document;
use App\Document\Log;
use App\Document\Price;
use App\Document\Revision;
use App\Repository\ProductRepository;
use Doctrine\ODM\MongoDB\Mapping\Annotations as MongoDB;
use Gedmo\Mapping\Annotation as Gedmo;
use Money\Money;
#[MongoDB\Document(collection: 'products', repositoryClass: ProductRepository::class)]
#[Gedmo\Loggable(logEntryClass: Log::class)]
#[Gedmo\Revisionable(revisionClass: Revision::class)]
class Product
{
#[MongoDB\Id(strategy: 'UUID')]
private ?string $identifier = null;
#[MongoDB\EmbedOne(targetDocument: Price::class)]
#[Gedmo\Versioned]
private ?Price $price = null;
public function getId(): ?string
{
return $this->identifier;
}
public function getPrice(): ?Money
{
if (isset($this->price)) {
return $this->price->getPrice();
}
return null;
}
public function setPrice(?Money $price): static
{
$this->price = null;
if ($price instanceof Money) {
$this->price = new Price($price);
}
return $this;
}
}
App/Document/Price.php
<?php
namespace App\Document;
use Doctrine\ODM\MongoDB\Mapping\Annotations as MongoDB;
use Gedmo\Mapping\Annotation as Gedmo;
use Money\Currency;
use Money\Money;
#[MongoDB\EmbeddedDocument]
class Price
{
#[MongoDB\Id(strategy: 'UUID')]
private ?string $identifier = null;
#[MongoDB\Field(type: 'decimal128')]
#[Gedmo\Versioned]
private ?string $amount = null;
#[MongoDB\Field(type: 'string')]
#[Gedmo\Versioned]
private ?string $currency = null;
public function __construct(Money $price)
{
$this->setPrice($price);
}
public function getId(): ?string
{
return $this->identifier;
}
public function getAmount(): ?string
{
return $this->amount;
}
public function setAmount(string $amount): static
{
$this->amount = $amount;
return $this;
}
public function getCurrency(): ?string
{
return $this->currency;
}
public function setCurrency(string $currency): static
{
$this->currency = $currency;
return $this;
}
public function getPrice(): ?Money
{
if (isset($this->amount) && is_numeric($this->amount) && isset($this->currency) && $this->currency != '') {
$decimals = 2;
$amount = (string) ((float) $this->getAmount() * 10 ** $decimals);
return new Money($amount, new Currency($this->currency));
}
return null;
}
public function setPrice(Money $price): static
{
$decimals = 2;
$amount = (string) ((float) $price->getAmount() / 10 ** $decimals);
$this->amount = $amount;
$this->currency = $price->getCurrency()->getCode();
return $this;
}
}
It works with the above changes and revision data gets set to:
{
"price": {
"amount": "670",
"currency": "GBP"
}
}
Without, it gets set to:
{
"price": []
}
Similarly with the log data.
The difference I notice is in the database they're all strings for loggable, whereas for revisionable they are the correct field type.
I'll play with that a bit and see what I can come up with.
The difference I notice is in the database they're all strings for loggable, whereas for revisionable they are the correct field type.
That's the other big change in this new extension. With loggable, it would just take the values from the objects and serialize the array. In the new code, I'm going through the Type::getType($type)->convertToDatabaseValue($value); APIs to log everything as its database representation (so no more serialized DateTime objects in the log data, as an example, it'll be a formatted string).
OK, I think I have this back to not requiring the revisionable attribute (or similar config in XML/YAML) on embedded models. The fields that should be versioned still need to be configured as such, but that looks to be the same with the current loggable extension so this should be OK.
Fantastic, that appears to work perfectly... for revisionable.
Which in theory should be fine, except I think the situation for embeds is:
- ✔️ Revisionable + ORM
- ✔️ Loggable + ORM
- ✔️ Revisionable + ODM
- ❌ Loggable + ODM
As an embedded document with versioned attributes gives the error:
Class must be annotated with Loggable annotation in order to track versioned fields in class - App\Document\Price
I get that it's probably kind of out of scope to add features to loggable in this PR, but as it's reusing the versioned attribute, and loggable and revisionable probably need to exist in tandem for a period of time, what would you suggest as the best resolution?
My one line change to isClassAnnotationInValid() might not be the full solution, but it does resolve it for my use-case.
On second thought, what you're probably expecting to happen is that the two exist in tandem in code only, and aren't actually used together. Meaning ODM embed's aren't supported by loggable (and probably never were), but are supported by revisionable, so you'd have to migrate, disabling loggable in the process.
In my mind, the two extensions can live side-by-side (which also gives a way to migrate data across if you need to, like the hacked together gist in the OP shows can be done), but a model shouldn't be both loggable and revisionable at the same time. That does get trickier with embedded models because of the behavioral differences between object managers, but if tweaking the loggable extension makes the migration more practical, then we should do that as well.
As a separate PR (because it sounds like the limitations with loggable and the ODM are already there and addressing that shouldn't rely on this PR landing anytime soon), I'd say to make the changes you're suggesting in the mapping drivers for the loggable extension, take the MongoDBODMMappingTestCase from this PR, and set up a LoggableMongoDBODMMappingTest (similar to the RevisionableMongoDBODMMappingTest in this PR) and all the relevant fixtures to make sure the mapping behaviors are working as expected with the ODM.
@mbabker Hey 👋 Great work on this new extension!
Do you have an indication of this PR's status, or an idea when it could get merged?
In my project we'll be needing something like this soon. And because Loggable is getting end-of-life, I'm following this PR curiously. It would be great if we could start with Revisionable, so we won't have to do any migration in the future.
I hope you don't mind me asking this directly 😊 I've sent a contribution through GitHub Sponsors for your work on this!
I hope you don't mind me asking this directly 😊 I've sent a contribution through GitHub Sponsors for your work on this!
Thank you!!!
Do you have an indication of this PR's status, or an idea when it could get merged?
I've gone ahead and marked the PR ready for review. We've been sitting on this for a bit, and it's gotten some feedback in passing, which has helped get it into a pretty good state overall. The one rough edge is probably the cases dealing with embedded models from a few comments back for folks who do need to migrate and have their apps support both loggable and revisionable for different models (i.e. an embedded Money object used in a loggable Product model and a revisionable Order model), and that can be iterated on without needing this PR merged since I think that's entirely an improvement in the loggable extension.
So I think we just need some good real-world testing on this PR and we can look at shipping it.
Not sure if bug or feature but I can't have have only #[Loggable] + #[Versioned] in an entity but it forces me to use both #[Loggable] + #[Revisionable] + #[Versioned]
Class "App\Entity\VendorOrder" has "Gedmo\Mapping\Annotation\Versioned" annotated fields but is missing the "Gedmo\Mapping\Annotation\Revisionable" class annotation.
Not sure if bug or feature but I can't have have only
#[Loggable] + #[Versioned]in an entity but it forces me to use both#[Loggable] + #[Revisionable] + #[Versioned]Class "App\Entity\VendorOrder" has "Gedmo\Mapping\Annotation\Versioned" annotated fields but is missing the "Gedmo\Mapping\Annotation\Revisionable" class annotation.
I'd need to see an entity to look at this more. The loggable mapping didn't change any and the revisionable mapping only looks for the Revisionable and Versioned attributes.
Not sure if bug or feature but I can't have have only
#[Loggable] + #[Versioned]in an entity but it forces me to use both#[Loggable] + #[Revisionable] + #[Versioned]Class "App\Entity\VendorOrder" has "Gedmo\Mapping\Annotation\Versioned" annotated fields but is missing the "Gedmo\Mapping\Annotation\Revisionable" class annotation.I'd need to see an entity to look at this more. The loggable mapping didn't change any and the revisionable mapping only looks for the
RevisionableandVersionedattributes.
# services.yaml
gedmo.mapping.driver.attribute:
class: Gedmo\Mapping\Driver\AttributeReader
gedmo.listener.revisionable:
class: Gedmo\Revisionable\RevisionableListener
tags:
- { name: doctrine.event_listener, event: 'onFlush' }
- { name: doctrine.event_listener, event: 'loadClassMetadata' }
- { name: doctrine.event_listener, event: 'postPersist' }
calls:
- [ setAnnotationReader, [ '@gedmo.mapping.driver.attribute' ] ]
# doctrine.yaml
orm:
# ...
entity_managers:
default:
# ...
mappings:
# ...
gedmo_loggable:
type: attribute
prefix: Gedmo\Loggable\Entity
dir: "%kernel.project_dir%/vendor/gedmo/doctrine-extensions/src/Loggable/Entity/"
alias: GedmoLoggable # (optional) it will default to the name set for the mappingmapping
is_bundle: false
revisionable:
type: attribute
alias: GedmoRevisionable
prefix: Gedmo\Revisionable\Entity
dir: "%kernel.project_dir%/vendor/gedmo/doctrine-extensions/src/Revisionable/Entity"
is_bundle: false
# stof_doctrine_extensions.yaml
stof_doctrine_extensions:
default_locale: en_US
orm:
default:
timestampable: true
blameable: true
loggable: true
sortable: true
<?php
declare(strict_types=1);
namespace App\Entity;
use App\Repository\VendorRepository;
use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Mapping as ORM;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Gedmo\Mapping\Annotation\Loggable;
use Gedmo\Mapping\Annotation\Versioned;
#[Loggable]
#[ORM\Table(name: 'vendors')]
#[ORM\Entity(repositoryClass: VendorRepository::class)]
class Vendor
{
#[Column(name: 'vendor_id', type: Types::INTEGER)]
#[Id]
#[GeneratedValue(strategy: 'IDENTITY')]
private ?int $vendorId = null;
#[Versioned]
#[Column(name: 'name', type: Types::STRING, length: 255, unique: true)]
private string $name;
public function getVendorId(): ?int
{
return $this->vendorId;
}
public function getName(): string
{
return $this->name;
}
public function setName(string $name): void
{
$this->name = $name;
}
}
⚠️ doesn't work but errors with
Class "App\Entity\Vendor" has "Gedmo\Mapping\Annotation\Versioned" annotated fields but is missing the "Gedmo\Mapping\Annotation\Revisionable" class annotation.
If I add #[Revisionable] (also to all other entities having #[Loggable]) then (both of them) works:
<?php
declare(strict_types=1);
namespace App\Entity;
use App\Repository\VendorRepository;
use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Mapping as ORM;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Gedmo\Mapping\Annotation\Loggable;
use Gedmo\Mapping\Annotation\Revisionable;
use Gedmo\Mapping\Annotation\Versioned;
#[Loggable]
#[Revisionable]
#[ORM\Table(name: 'vendors')]
#[ORM\Entity(repositoryClass: VendorRepository::class)]
class Vendor
{
#[Column(name: 'vendor_id', type: Types::INTEGER)]
#[Id]
#[GeneratedValue(strategy: 'IDENTITY')]
private ?int $vendorId = null;
#[Versioned]
#[Column(name: 'name', type: Types::STRING, length: 255, unique: true)]
private string $name;
public function getVendorId(): ?int
{
return $this->vendorId;
}
public function getName(): string
{
return $this->name;
}
public function setName(string $name): void
{
$this->name = $name;
}
}
I can try to set up a full reproducer next week if this isn't enough to figure it out
edit: not registering gedmo.listener.revisionable makes the old code work with this branch (but then the revisionable stuff won't obviously work)
The only thing I can think of without building a full reproducer is that the config being passed by reference is causing an unexpected side-effect, adding a dump($config) before this line would confirm that.
The only thing I can think of without building a full reproducer is that the config being passed by reference is causing an unexpected side-effect, adding a
dump($config)before this line would confirm that.
Apparently, I can't use the same config key name (versioned) in both extensions since it looks like the ExtensionMetadataFactory::getExtensionMetadata() method doesn't pass a "clean" config array into the mapping drivers (no idea if I'd call this a bug or a feature at this point, but here we are).
I'll have to think about ways to improve this because the issue's really a symptom of a bigger design concern overall.