Problem/Motivation
Config entities have a baked-in concept of synchronization, which is used to inform hook implementations that the entity is being changed as part of config import process.
The Workspace uses a very similar concept, it sets a _isReplicating
temporary flag on the entity to inform hook implementations that the entity is being changed as part of publishing the revisions of a non-default workspace to Live.
Proposed resolution
Introduce a SynchronizableInterface
and provide a base implementation in ContentEntityBase
.
This could potentially be used also by other modules that import content entities into a site, like default_content, etc.
Remaining tasks
Discuss, review.
User interface changes
Nope.
API changes
API additon: content entities have two new methods available: setSyncing()
and isSyncing()
.
Data model changes
Nope.
Comment | File | Size | Author |
---|---|---|---|
#32 | 2985297-32.patch | 11.69 KB | hchonov |
#19 | interdiff-19.txt | 4.45 KB | amateescu |
#19 | 2985297-19.patch | 11.68 KB | amateescu |
#2 | 2985297.patch | 8.85 KB | amateescu |
Comments
Comment #2
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis is what I had in mind.
Comment #3
tstoecklerActually this already came up a long time ago in the context of migration. See #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating), in particular my patch in #19 over there. It's conceptually very similar, but very nice addition here that it actually allows us consolidate semantics/naming. So tentative +1 (tentative merely because I haven't thought about this or #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) very deeply in a while and am also not really accustomed with the Workspace module)
Comment #4
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNice, we already have a use-case besides workspaces :D
Comment #5
hchonovWith that patch we'll have the same property and methods both implemented in ContentEnitityBase and ConfigEntityBase. I think it would be better to move the property and the methods to the least common ancestor instead - Entity.
Comment #6
hchonovDo we want to call the entity hooks when saving an entity in a sync state? Probably not?
Comment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat would make the new interface pointless. The idea is that you can combine all these interfaces as you need them, instead of sticking everything on
Entity
andEntityInterface
.We absolutely need to call the hooks. As documented in the
isSyncing()
method, it is the responsibility of the developer to do or not do certain changes to entity based on this flag.Comment #8
hchonovWhat is wrong in having basic methods to live in the Entity class given they have 1:1 implementation both for content and config entities?
We could implement the new interface SynchronizableInterface in Entity next to the EntityInterface instead of making the EntityInteface extend the new SynchronizableInterface.
Comment #9
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedJust like how every entity type doesn't have to be revisionable, translatable, publishable, etc., they don't have to be synchronizable either..
Comment #10
hchonovHmm, I think there is some misunderstanding here.
I don't get it what is the difference between implementing the SynchronizableInterface in ContentEntityBase and ConfigEntityBase vs implementing the SynchronizableInterface only in the Entity class?
Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOur Entity object is quite abstract and I don't think we should force all of its subclasses (which might be something different than content or config entities) to have the concept of being synchronizable.
Comment #13
hchonovNow I understand what you mean, however I see it exactly the other way around - by placing the methods in the Entity class even entities which aren't content or config entities will profit by this as they will automatically receive the new methods. We don't force them to anything, as they don't have to implement those methods, because they are already implemented in the Entity class. I don't see any harm in doing so.
Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAs far as I see it, that would break the principle of composition, that's why I would prefer to keep them out of the Entity class.
Comment #15
hchonovCould you please elaborate on this? We use inheritance and not composition for entities and this is also what we suggest in https://www.drupal.org/core/d8-bc-policy :
Using exactly your arguments one might say that adding the methods to ContentEntityBase makes and forces all content entities to be synchronizable, which is exactly the same for entities that are neither content nor config entities. To cover that case and your arguments we would need to do the same as with e.g. the TranslatableInteraface - we need to specify in the entity type through the entity annotation whether an entity type is synchronizable or not. So this is one thing that we would have to do no matter which implementation we choose.
Additionally if we decide not to implement the methods and the interface in the Entity class, then probably it makes sense to offer a trait, which contains the property and both methods.
Comment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAll the optional interfaces like
EntityOwnerInterface
,EntityPublishedInterface
,EntityChangedInterface
, etc. seem to contradict you.There's no 1-1 relationship between
Entity
and this newSynchronizableInterface
.That's the design decision taken by this patch.
Having a way to define in the entity definition whether the entity type is translatable or not is there so people can enable/disable translatability using a hook, we don't want to offer that kind of configurability here.
Yes, we can use a trait for that.
Comment #17
hchonovWait, wait. This is not composition. I was confused as you've mentioned the composition principle, which is something else - https://en.wikipedia.org/wiki/Composition_over_inheritance . So I still don't get it what is the principle you are talking about?
This was meant as an example, that we use inheritance and not composition.
Now I am confused, a lot. So you are against forcing non-content and non-config entities to be synchronizable, but you want to force all content entities to be synchronizable? So if a I have an entity which extends from Entity, but it is neither config nor content entity and for some argument I don't want it to be synchronizable, then it would not be, but if I don't want some of my content entities to be synchronizable because of the same argument, then there is nothing I can do about this? I am sorry, but I really don't see the point here.
Comment #18
hchonovIn the name of progress let's do it your way. Later we still could move the implementation one level up.
3 remaining tasks here -
getProperties
inContentEntityCloneTest::testEntityPropertiesModifications()
, which is why the test isn't failing but it should've failed, because we still have to ensure that the property is shared among entity translation objects.Comment #19
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe point is that core has decided that all content and config entities are synchronizable, while some custom or contrib module can decide that an entity object which extends from
\Drupal\Core\Entity\Entity
doesn't need this capability. That's what I meant with "the design decision taken by this patch" and I don't understand what's so confusing about this.I extracted the methods and the property in a trait, but that's just a few lines of code and doesn't really buy us too much IMO.
Because it was like that in the original implementation in
\Drupal\Core\Config\Entity\ConfigEntityBase
:) Very good point about ensuring that it is properly shared among translation objects, changed it to protected and updated the needed code inContentEntityBase
.Comment #20
hchonov@amateescu, thank you. Looks good to me now.
Comment #22
hchonovWe need a change record for the new feature.
Comment #23
Wim LeersCorrect?
Comment #24
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedYup.
Comment #25
Wim Leers👍
Comment #26
catchThis needs a change record for the API addition. Also do we need test coverage for the feature as applied to content entities?
Comment #27
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAdded a CR: https://www.drupal.org/node/2988067
I don't think we can write any meaningful test coverage for content entities because each use-case is highly specific to their implementation. For example, we have test coverage for the use of content entity synchronization in the Workspaces module, but what Workspaces sees as acceptable changes to an entity during the synchronization process doesn't imply that other modules or hook implementations need to behave in the same way.
Comment #28
hchonovThis is correct.
Right after the patch here is committed, we can add the test coverage for the already provided patch in #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating). I think this would be enough for the core. @catch, is it fine for you that this feature gets test coverage in the other issue, where we have the real use case?
Comment #29
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWell.. Workspace's use case is real and tested too, as demonstrated by the last two hunks of this patch :)
Comment #30
hchonovSure :), I've only meant that there the test will be more explicit - set the synching status to TRUE, update the entity and save it, ensure the changed timestamp has not changed :).
Comment #32
hchonovOnly a re-roll.
Comment #33
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI was posting the same patch concurrently :) The reroll looks good!
Comment #35
larowlanCommitted cb5774f and pushed to 8.7.x.
Published the change record
Comment #37
joseph.olstadI used this setSyncing(TRUE) during a content migration/content post migration processing , and it seems to work as we wanted to avoid creating unnecessary revisions during our post migration processing.
However, looking at the change record, not much explanation as to exactly what this option fully means,
is there other situations were I might want to use setSyncing? like if I am changing all translations of a node at once to syncronise moderation state and content with the current revision?
It would be helpful to have a more descriptive explanation of what setSyncing might be useful for.
here is the change record
https://www.drupal.org/node/2988067
module developer documentation would be helpful, specifically with how this option might be used with translation and content moderation states.
Thanks
Comment #38
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSee #3057483: [PP-2] Better describe how SynchronizableInterface should be used for content entities.