Add EntityChangedInterface to allow entities with "changed" field to be properly cached

Also related: #2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash.

EDIT: fixed issue reference.

This is requirement for data migration too so this is essential for us to put migrate module upgrade path in core for d9.

Got it.

I added a field along with some basic accessor/setter functions. I can provide more (or less) functionality if needed, but I thought I'd throw this out there.

I think it should go in the Entity system. Also, a few entity types (node, comment etc.) already have this field under another name ("changed"), they must be consolidated.

See #1907960-353: Helper issue for "Comment field", where I suggested a separate interface: EntityChangedInterface.

Most content entities already have such a field, it's just not unified.

Yeah that way it sounds much more feasible at this late time.

While this interface may be useful in itself, it is not really useful without actual data stored. Nodes have a changed timestamp but I believe most other entities (eg. menu items, users, etc) don't have this data. So we'd need to alter the schema on them and get this data stored on them for this to actually be useful. Then you'll see nodes have this in the 'updated' column/property, so if we want to introduce this in a different way elsewhere, we may want to rename the node column also. That may have wide ripple effects.

As for the actual code, time() should be replaced with REQUEST_TIME AFAIS.

With an interface, each entity that has this field can have its how getChangedTime() method and the patch is minimal.

To add a new column for other entities or rename columns, the patch will be much bigger and is more subtle to go in.

@jcisio: Yeah, well, this issue in itself adds code but does not make any of the returned values trustworthy unless its implemented. At least some error condition should be returned by default IMHO so if this is not implemented on an entity, you don't get some fake data. (Assuming 0 may be valid data).

The patch/code has nothing to with my suggestion of a separate interface and can be ignored :)

The point of the interface is that it's optional and not tied to a specific field name.
So if you'd use it, you would do:

if ($entity implements EntityChangedInterface) {
  $updated = $entity->getChangedTime()
}

As for support, node has changed, comments too, terms don't have anything atm, users have created, access and login but no changed, files have timestamp (which is changed), custom blocks don't have any created or changed timestamps.

Getting the interface in and implementing it in this issue for entity types that already have this information is a small API addition, without having to do any storage or other API changes. And then we can open follow-ups to add support for this to other entity types. custom blocks could be very interesting for example, so that they can be cached as long as possible.

Right, I agree it may be followups, all I wanted to point out was that the interface in itself does not really help and that if we make the entity base class implement it, then the default implementation should return some kind of error condition, exception or whatnot IMHO. Just getting false data and then working with it like it was a real changed timestamp sounds like an issue.

@Berdir: here is a reworked version. Better? :)

Opened a META issue for above this one at #2074191: [META] Add changed timestamp tracking to content entities. Started expanding implementation of changed timestamps at #2074193: Add changed time tracking to custom blocks.

Yes, that looks good to me.

@Berdir, et. al: passes green :) looks good? Anything else to do here? :)

Why exactly don't we add getChangedTime() on all content entities? i.e. why a new interface and not just modify ContentEntityInterface? I fear we'll end up with most content entities not implementing this interface, precisely because it is optional, even though it costs close to zero effort to support.
If it's not on all content, then migrate module (see #2) also won't work very well I think. Any content entity that is stored in the DB, might as well be saved at a later point.

Gábor pointed out (in a private chat about this) that Berdir's position is that this may be a feature that not all content entities support. e.g. contact messages.

So I analyzed the code for contact messages (MessageInterface, Message et al.) and AFAICT the message entity is NEVER saved. So it would not end up in the DB and would not be rendered. Meaning that getChangedTime() could not even be called if somebody wanted to: the entity object itself exists only very briefly.
It could only be called by MessageFormController::save(), which is the very short duration during which it exists (that calls drupal_mail(), not $message->save().
Furthermore, implementing getChangedTime() is not a problem at all: precisely because it's an ephemeral object (i.e. exists only during the request), Message::getChangedTime() should just return REQUEST_TIME, which is correct.

What am I missing? :)

I support the interface because it keeps patches smaller. When every issue in #2074191: [META] Add changed timestamp tracking to content entities adds support for getChangedTime, we can simply then make a small patch to remove the interface and add that method into the base class.

@jcisio: It is easy to add support and then expand the interface too. The issues don't currently use the interface (but extend the interfaces of specific entities instead).

Assuming the benefits in the issue summary are correct, I've no idea why we wouldn't add this to the base class. Could the issue summary please be updated with what the advantages of an interface instead are? It seems like not adding it to the base class will just make code that deals with entities have to do a lot of special-handling.

Because not all core entities have the necessary data to implement this interface correctly. I'd rather have an optional method/interface than require to implement a method that might return NULL.

The code complexity doesn't seem to be different, it's "$entity instanceof EntityChangedInterface" vs. "$entity->getChangedTime() !== NULL". I prefer the first.

That is, unless you want to make having a changed date mandatory for any content entity, not sure if that's a good thing.

The method won't be useful for migrate anyway, we'd need a changed entity key/field definition for that. (Might also be useful as an optional definition).

Should we also add the entity key then? :)

That is, unless you want to make having a changed date mandatory for any content entity

That's exactly what I asked :)

Regardless of the data migration use case; it makes sense for any piece of content (i.e. content entities) to be able to cache them on the client side. Thus we need a way to invalidate the client-side cache. Hence we need getChangedTime() on all content entities.

not sure if that's a good thing

Can you be more specific about why that may be a bad thing? :) Again: what am I missing? :) (See #23.)

Well, I don't have very specific arguments...

* My main thought was that adding an optional feature is a relatively simple API addition that doesn't break anything and should be fairly easy to get in ( the only problematic argument would be "why is this not required" ;))
* Enforcing it however is an API and storage change, so the impact is much bigger and you need to convince core maintainers that it's worth it
* The scope of content entities is broadened in #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface (fieldable entity => content entity), so I'm not sure about enforcing it for all of them. For example, i have more than one entity type in contrib modules related to e.g. logging/activity messages that are by design not change-able. But they can have fields so with that issue, they will be content entities.
* A separate interface could still be implemented by default in ContentEntityInterface once the opened meta issue is resolved, and it would allow to apply that concept also to non-entities.

I'm personally interested in having the ability to figure out the last changed date of an entity, I'm not really interested in enforcing it but I'm also not against it as long as it's configurable/flexible enough to not get in the way (e.g. a write once entity could return the created date).

(Just had a chat about this with berdir.)

My main concern is this:

I fear we'll end up with most content entities not implementing this interface, precisely because it is optional, even though it costs close to zero effort to support.

berdir's proposing a separate EntityChangedInterface because we can still merge it with ContentEntityInterface at a later point.

I would prefer to see it happen now rather than in an uncertain future, where we end up with yet another interface that is making the overall DX more fragmented/complex.

The problem is entities that are write-once, where the solution would indeed be precisely what berdir says: a write-once entity could return the created date. Which brings us to the big problem of adding ContentEntityInterface::getChangedTime(): not all entities even have a created date. For example: Taxonomy Terms — clearly content entities (so they should have getChangedTime(), but they don't even store the created date. So in order to do what I'm advocating, we would need to adjust the schemas of all content entities, to include a "changed" timestamp for editable entities and a "created" timestamp for non-editable (write-once) entities.

Then again, that's what Gábor is already doing with #2074191: [META] Add changed timestamp tracking to content entities and all those subissues. So… is it worth waiting until all that is done, so we can modify ContentEntityInterface and have a consistent, simple DX, or are there still reasons for creating EntityChangedInterface?

Just wanted to disagree with Wim here about DX. Having to implement a schema field, plus a method, plus an entry in baseFieldDefinitions() for a field that I'm never going to use is more of a pain than implementing an optional interface. I would actually like to see more composable interfaces for entities, not less.

There's no reason that Migrate can't track its own changed field for entities that don't support it.

Plus, there's no reason that Config entities wouldn't be able to support this.

Edit:
I have no problem with all of core implementing said interface, and documenting on the interface/elsewhere that if you want your entity to be super-awesome-cacheable, you need this.

Plus, there's no reason that Config entities wouldn't be able to support this.
Yes, this definitely should stay an optional interface. Hardcoding is what led to #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface.

#32/#33: Much thanks to the both of you, that's the feedback I was looking for :)

Back to reviewing the patch in #18. What is next?

The only remark I have for now is that I'd like to see something like what twistor suggested in #32: provide a bit more practical documentation on the interface.

+++ b/core/lib/Drupal/Core/Entity/EntityChangedInterface.php
@@ -0,0 +1,23 @@
+/**
+ * Defines a common interface for entity change timestamp tracking.
+ */
+interface EntityChangedInterface {

I think it might be useful to have additional text below the first line, with something along the lines of "This can be used for example for invalidating client-side caches and concurrent editing locking."

Reworked the suggested text a bit. Wim IMHO your suggested text may be misunderstood as those *implementing* this interface may invalidate client side caches or do something with concurrent edit locking, but that is not the case. Any other suggestions? :)

Nope :)

Is there something else that needs to happen here?

I don't think anything else is needed, no.

Let's get it done!

Ok cool. I still find this approach a bit bizarre but twistor in #32 lays out some good points.

Committed and pushed to 8.x. Thanks!

This will need a change notice.

Added change notice at https://drupal.org/node/2079207