Introduce the Revisionable extension

[mbabker]

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 WrapperInterface implementations (and @method annotated 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 Revisionable attribute instead of Loggable, and reuses the existing Versioned attribute; 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/>
• Fully typed to the extent PHP 7.4 allows

[simoheinonen]

Having no username set in the RevisionableListener doesn't work because ->setUsername() expects a string even though it is nullable in the database

[mbabker]

Having no username set in the RevisionableListener doesn'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]

Codecov Report

Attention: Patch coverage is 85.10638% with 84 lines in your changes missing coverage. Please review.

Project coverage is 78.92%. Comparing base (211b6fe) to head (922d934).

Files with missing lines	Patch %	Lines
src/Revisionable/RevisionableListener.php	85.61%	20 Missing ⚠️
src/Revisionable/Mapping/Driver/Xml.php	82.35%	12 Missing ⚠️
src/Revisionable/Mapping/Driver/Yaml.php	80.39%	10 Missing ⚠️
src/Mapping/Annotation/Revisionable.php	25.00%	9 Missing ⚠️
src/Revisionable/Mapping/Driver/Attribute.php	84.61%	8 Missing ⚠️
...ble/Document/MappedSuperclass/AbstractRevision.php	80.00%	6 Missing ⚠️
...ionable/Document/Repository/RevisionRepository.php	91.52%	5 Missing ⚠️
src/DoctrineExtensions.php	0.00%	4 Missing ⚠️
...nable/Entity/MappedSuperclass/AbstractRevision.php	86.66%	4 Missing ⚠️
...isionable/Entity/Repository/RevisionRepository.php	93.33%	4 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2825      +/-   ##
==========================================
+ Coverage   78.53%   78.92%   +0.39%     
==========================================
  Files         168      181      +13     
  Lines        8790     9353     +563     
==========================================
+ Hits         6903     7382     +479     
- Misses       1887     1971      +84     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[simoheinonen]

What's the status of this PR? Anything I can do to help/test? Would like to see it merged soon!

[mbabker]

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).

[mustanggb]

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 void returns - 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 StofDoctrineExtensionsBundle integration - Biggest stumpling block so far as event listeners aren't triggering at all.
• No documentation/examples - i.e. doc/revisionable.md and updated doc/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

[mbabker]

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 StofDoctrineExtensionsBundle integration - 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.md and updated doc/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.

[mustanggb]

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)

[phansys]

Could we use a stricter condition around $value?

[mbabker]

Maybe a !== null would work in this branch? It'd be another one of those "we gotta test it thoroughly" things, and TBH there's only a very basic "does it work without erroring" smoke test for the revert() capabilities so we'd need to build out some good test fixtures here.

[mbabker]

This is where me not knowing the MongoDB ODM well doesn't help any.

1 "isSingleValuedAssociation"
2 "67ce41458a6433683e0c45e6"

This is the only time the branch gets reached in the RevisionableDocumentTest. Looking at the comment fixture, it looks like this is going to be the ReferenceOne relation to the article, so I guess $value in this case is the document identifier? If that's the case then this could most likely work as a !== null check, assuming that it goes from a non-null to a null value when a non-required reference is removed.

[phansys]

Friendly ping @franmomu 🙏

Based on your experience, maybe you have a chance to help with this.

[phansys]

We should add a keyword for this new extension at composer.json.