Problem/Motivation

In order to effect timely and useful caching and cache busting, we need to know that last time an entity was updated. Given a consistent means to check updated time we can know, for instance, when assets have gone stale in a client-side application.

This one small and consistent piece of information will allow us to build robust caching graphs across dependent entities and invalid their caches more timely and precisely.

Proposed resolution

Add the updated time to a field on the base entity class.

Remaining tasks

Add the field to a base entity class.

User interface changes

None.

API changes

None.

#2074191: [META] Add changed timestamp tracking to content entities
#2044579: [meta] Supporting single page application development on Drupal Core.

Original report by jessebeach

Files: 
CommentFileSizeAuthor
#35 interdiff.txt548 bytesGábor Hojtsy
#35 entity-change-interface-35.patch4.05 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,414 pass(es).
[ View ]
#18 entity-change-interface.patch3.87 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,533 pass(es).
[ View ]
#8 updated-time-2044583-8.patch1.46 KBjlindsey15
PASSED: [[SimpleTest]]: [MySQL] 57,550 pass(es).
[ View ]
#6 updated-time-2044583-6.patch1.46 KBjlindsey15
FAILED: [[SimpleTest]]: [MySQL] 57,118 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#4 updated-time-2044583-4.patch2.22 KBjlindsey15
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Wim Leers’s picture

moshe weitzman’s picture

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

jlindsey15’s picture

Assigned:Unassigned» jlindsey15

Got it.

jlindsey15’s picture

Status:Active» Needs review
StatusFileSize
new2.22 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, updated-time-2044583-4.patch, failed testing.

jlindsey15’s picture

Status:Needs work» Needs review
StatusFileSize
new1.46 KB
FAILED: [[SimpleTest]]: [MySQL] 57,118 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, updated-time-2044583-6.patch, failed testing.

jlindsey15’s picture

StatusFileSize
new1.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,550 pass(es).
[ View ]
jlindsey15’s picture

Status:Needs work» Needs review
jcisio’s picture

Component:cache system» entity system
Status:Needs review» Needs work

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.

Berdir’s picture

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.

jcisio’s picture

Title:Add an Updated Time field to all entities» Add EntityChangedInterface to allow entities with "changed" field to be properly cached

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

Gábor Hojtsy’s picture

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.

jcisio’s picture

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.

Gábor Hojtsy’s picture

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

Berdir’s picture

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:

<?php
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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new3.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,533 pass(es).
[ View ]

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

Gábor Hojtsy’s picture

Issue tags:+sprint, +Spark

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.

Berdir’s picture

Issue tags:-sprint, -Spark

Yes, that looks good to me.

Gábor Hojtsy’s picture

Issue tags:+sprint, +Spark
Gábor Hojtsy’s picture

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

Wim Leers’s picture

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? :)

jcisio’s picture

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.

Gábor Hojtsy’s picture

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

webchick’s picture

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.

Berdir’s picture

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

Gábor Hojtsy’s picture

Should we also add the entity key then? :)

Wim Leers’s picture

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

Berdir’s picture

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

Wim Leers’s picture

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

twistor’s picture

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.

tim.plunkett’s picture

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.

Wim Leers’s picture

#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."

Gábor Hojtsy’s picture

StatusFileSize
new4.05 KB
PASSED: [[SimpleTest]]: [MySQL] 58,414 pass(es).
[ View ]
new548 bytes

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? :)

Wim Leers’s picture

Nope :)

Is there something else that needs to happen here?

Gábor Hojtsy’s picture

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

Wim Leers’s picture

Status:Needs review» Reviewed & tested by the community

Let's get it done!

webchick’s picture

Title:Add EntityChangedInterface to allow entities with "changed" field to be properly cached» Change notice: Add EntityChangedInterface to allow entities with "changed" field to be properly cached
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

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.

Gábor Hojtsy’s picture

Priority:Critical» Normal
Status:Active» Fixed
Issue tags:-Needs issue summary update, -Needs change record, -sprint

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

jcisio’s picture

Title:Change notice: Add EntityChangedInterface to allow entities with "changed" field to be properly cached» Add EntityChangedInterface to allow entities with "changed" field to be properly cached

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

Anonymous’s picture

Issue summary:View changes

Add META issue.