#3562989: Implements RevisionLogInterface for Instance entity show some issues in our current management of "saved" data:
- we are storing data identical to, so duplicated of, the ones already stored by displays in config and content storage. This is useless, take storage space and error prone. What is happening if the data are diverging? how to reconciliation?
- this
savedfield has no equivalence in Revisions API
So, lets fix it before adopting Revisions API.
Proposed resolution
Instance entity must be aware of the buildable plugin
Add a buildable string base field to InstanceEntity. If an instance entity is created by a DisplayBuidable plugin, the plugin ID is stored in the base field.
Add PublishableInterface::getBuildablePlugin(): the plugin ID with the already stored contexts must be enough to load any DisplayBuidable plugin from any Instance entity.
Use the new mechanism in event subscribers
Thanks to the new ::getBuildablePlugin(), the logic of each DisplayBuilderEvents::ON_SAVE subscribers (1 per sub-module) become strictly identical, so we can do 3 simplifications:
- Merge them into a single subscriber service in main module.
DisplayBuildable::getContextRequirement()can become protected, rely on plugins attributes, and be removed from the interface.- We can pass the Instance entity coming from the controller in
DisplayBuilderEventsinstead of recretaing it from Instance ID.
We can do even more: remove the event all together ! We already have the instance, so we already know the buildable plugin, so we can publish.
Move the publication logic to display buildable plugins
In:
Instance::saveIsCurrent()check the hash of the buildable sources and compareInstance::hasSave()becomes::getPublishedHash()
Then:
- Remove
savebase field - Remove
Instance::setSave()
Remaining tasks
Leveraging the contexts may also be the opportunity to less rely on the "black magic" of parsing instance ID string. See: #3573905: Simplify DisplayBuildableInterface
Issue fork display_builder-3578469
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3578469-make-instance-entities
changes, plain diff MR !255
Comments
Comment #3
pdureau commentedWork in progress
Remaining:
DisplayBuildableInterface::createFromInstanceEntity()implementations work only with contexts instead of parsing instance entityt IDInstancePublishingTestComment #4
pdureau commentedComment #5
pdureau commentedWorks about context has been moved to its own issue #3579299: Tidy context management
Comment #6
pdureau commentedDo we store the instance ID in a field or do we rely on
ContextAwarePluginManagerTrait::getDefinitionsForContexts()?Comment #7
pdureau commentedChained issues: #3562989: Implements RevisionLogInterface for Instance entity is rebasing the current issue which is rebasing #3579299: Tidy context management which is rebasing
1.0.xComment #8
pdureau commentedWe may also need to get rid of
profilecontent field in Instance entities.And what about
ApiController::revert()? There is some logic belonging toentity_view_overridebuildable plugin here. Do we introduce a "revertable" interface for buildable plugins? Maybe in an other issue.Comment #9
pdureau commentedComment #10
pdureau commentedComment #11
pdureau commentedComment #12
pdureau commentedComment #13
pdureau commentedWe are getting close ;)
TODO:
InstancePublishingTest,ApiPublishingControllerTest, playwright...RemoveInstanceInterface::getBuildablePlugin()?Follow-up:
profileIdfield from instance entity\Drupal\display_builder_entity_view\EventSubscriber\DisplayBuilderSubscriber::onRevert()Comment #14
pdureau commentedReady to review.
This MR is embedding #3579299: Tidy context management and embedded in #3562989: Implements RevisionLogInterface for Instance entity, which can be reviewed together or separately.
Comment #15
pdureau commentedTime goes by and Jean (@mogtofu33) clearly expressed the will to review all changes in a single MR, the one from #3562989: Implements RevisionLogInterface for Instance entity.
It is not necessary to keep this intermediary ticket in review.