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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
FileSize
8.85 KB

This is what I had in mind.

tstoeckler’s picture

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

amateescu’s picture

hchonov’s picture

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

hchonov’s picture

Do we want to call the entity hooks when saving an entity in a sync state? Probably not?

amateescu’s picture

I think it would be better to move the property and the methods to the least common ancestor instead - Entity.

That 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 and EntityInterface.

Do we want to call the entity hooks when saving an entity in a sync state? Probably not?

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.

hchonov’s picture

That 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 and EntityInterface.

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

amateescu’s picture

We could implement the new interface SynchronizableInterface in Entity next to the EntityInterface instead of making the EntityInteface extend the new SynchronizableInterface.

Just like how every entity type doesn't have to be revisionable, translatable, publishable, etc., they don't have to be synchronizable either..

hchonov’s picture

Just like how every entity type doesn't have to be revisionable, translatable, publishable, etc., they don't have to be synchronizable either..

Hmm, 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?

amateescu’s picture

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?

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

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hchonov’s picture

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

amateescu’s picture

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

hchonov’s picture

As far as I see it, that would break the principle of composition

Could 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 :

Where there is a 1-1 relationship between a class and an interface, inherit from the class.

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.

amateescu’s picture

We use inheritance and not composition for entities

All the optional interfaces like EntityOwnerInterface, EntityPublishedInterface, EntityChangedInterface, etc. seem to contradict you.

Where there is a 1-1 relationship between a class and an interface, inherit from the class.

There's no 1-1 relationship between Entity and this new SynchronizableInterface.

adding the methods to ContentEntityBase makes and forces all content entities to be synchronizable

That's the design decision taken by this patch.

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.

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.

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.

Yes, we can use a trait for that.

hchonov’s picture

We use inheritance and not composition for entities

All the optional interfaces like EntityOwnerInterface, EntityPublishedInterface, EntityChangedInterface, etc. seem to contradict you.

Wait, 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?

Where there is a 1-1 relationship between a class and an interface, inherit from the class.

There's no 1-1 relationship between Entity and this new SynchronizableInterface.

This was meant as an example, that we use inheritance and not composition.

adding the methods to ContentEntityBase makes and forces all content entities to be synchronizable

That's the design decision taken by this patch.

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.

hchonov’s picture

Status: Needs review » Needs work

In the name of progress let's do it your way. Later we still could move the implementation one level up.

3 remaining tasks here -

  1. Offer the implementation in a trait.
  2. Why is the property private? Can we convert it to protected?
  3. Because of the property being private it is not being returned for subclasses by the reflection method getProperties in ContentEntityCloneTest::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.
amateescu’s picture

Status: Needs work » Needs review
FileSize
11.68 KB
4.45 KB

I am sorry, but I really don't see the point here.

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

Offer the implementation in a trait.

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.

Why is the property private?

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 in ContentEntityBase.

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

@amateescu, thank you. Looks good to me now.

The last submitted patch, 2: 2985297.patch, failed testing. View results

hchonov’s picture

We need a change record for the new feature.

Wim Leers’s picture

  1. This is a pure in-memory flag.
  2. Hence it's never stored.
  3. From that, it follows that the API-First Initiative is not at all impacted.

Correct?

amateescu’s picture

Yup.

Wim Leers’s picture

👍

catch’s picture

Status: Reviewed & tested by the community » Needs review

This needs a change record for the API addition. Also do we need test coverage for the feature as applied to content entities?

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

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

hchonov’s picture

I don't think we can write any meaningful test coverage for content entities because each use-case is highly specific to their implementation.

This 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?

amateescu’s picture

Well.. Workspace's use case is real and tested too, as demonstrated by the last two hunks of this patch :)

hchonov’s picture

Well.. Workspace's use case is real and tested too, as demonstrated by the last two hunks of this patch :)

Sure :), 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 :).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2985297-19.patch, failed testing. View results

hchonov’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.69 KB

Only a re-roll.

amateescu’s picture

I was posting the same patch concurrently :) The reroll looks good!

  • larowlan committed cb5774f on 8.7.x
    Issue #2985297 by amateescu, hchonov: Generalize the concept of entity...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed cb5774f and pushed to 8.7.x.

Published the change record

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

joseph.olstad’s picture

I 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

Online documentation: Not done
Theming guide: Not done
Module developer documentation: Not done
Examples project: Not done
Coder Review: Not done
Coder Upgrade: Not done
Other: Other updates done

module developer documentation would be helpful, specifically with how this option might be used with translation and content moderation states.
Thanks

Sam152’s picture