Problem/Motivation

Terms don't have changed time tracking. See various reasons why this is a problem at #2074191: [META] Add changed timestamp tracking to content entities.

Proposed resolution

Add change time tracking to terms.

Remaining tasks

Needs upgrade path.

User interface changes

None.

API changes

Schema changed. New getChangedTime method added. This naming is already used on other entity types. See #2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached for unification of this.

#2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached
#2074191: [META] Add changed timestamp tracking to content entities

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +Needs upgrade path

Also needs upgrade path.

Status: Needs review » Needs work

The last submitted patch, term-changed-timestamp.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Updated patch as per findings in #2074193: Add changed time tracking to custom blocks. Should not define property on Term anymore but should define it is baseFieldDefinitions() array instead. Still needs upgrade path.

Wim Leers’s picture

For the upgrade path, what about this:

  • if the entity has a "created" timestamp, use that
  • otherwise: do something like variable_set('d8_update_time', REQUEST_TIME) when starting the D7 -> D8 upgrade process, then use that time for all entities that need a "changed" timestamp

I don't think we are able to generate better timestamps?

Gábor Hojtsy’s picture

Yeah, terms don't have any kind of timestamp... We can take the request time in update. It is pretty easy to update all entities at once with a query. Or are you advocating doing entity saves for each? That, especially given that we are changing almost all entities on your site including users and stuff with all these patches sounds like would take a time that is totally prohibitive. The update will just run forever.

Wim Leers’s picture

Yes, update all entities that are of entity types that don't have a "created" timestamp in one query. Saving each entity individually would be very painful.

Gábor Hojtsy’s picture

We can also do an update with an expression for those which have a created time to just copy that to the changed field :)

Status: Needs review » Needs work

The last submitted patch, term-changed-timestamp-3.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.89 KB

Now only the upgrade path fails remained, so here is the upgrade path :)

tstoeckler’s picture

Even though it's technically out of scope, can we add a 'created' timestamp as well, while we're at it. Having both seems to be the (emerging) standard, and since we have to provide an upgrade path etc. anyway, I think that would make sense.

Gábor Hojtsy’s picture

Issue tags: -sprint, -Spark

Yeah, well, uid would also be good, and uid/created/changed combined is useful for accountability, but I'd rather not go and expand the scope :( See lack of agreement in #1295148: Maintain common properties/statistics about entities.

Gábor Hojtsy’s picture

Issue tags: +sprint, +Spark
tstoeckler’s picture

Status: Needs review » Needs work
Issue tags: +Needs upgrade path tests

Yeah, OK, I guess.

The patch itself looks but needs upgrade path tests.

Gábor Hojtsy’s picture

@tstoeckler: ok, the prior patch failed in the upgrade test because it could not find the proper column. So if the patch with the upgrade path passes, it proves there was "some" coverage in the upgrade test :) What kind of coverage you are looking at. We cannot really tell which timestamp is the right one after update anymore, so just take a look if a != 0 timestamp is there?

Gábor Hojtsy’s picture

Totally did not want to remove the tag.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: -Needs upgrade path tests
FileSize
819 bytes
4.01 KB
3.58 KB

Yeah, something like that sounds good. I think that is more explicit than the fails in the previous patch, that don't relate to taxonomy module at all.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me :)

skyredwang’s picture

What is the chance of porting this patch to D7?

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.88 KB
2.2 KB

@skyredwang points in IRC to a very sensible missing piece. Views integration... IMHO we cannot just skip this.

skyredwang’s picture

Status: Needs review » Reviewed & tested by the community

I tested the new views support in #19, fields, sort, filter, argument all work.

Gábor Hojtsy’s picture

FileSize
818 bytes
5.99 KB

After #2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached landed, this should now use the interface proper. Should not change RTBC status, this is minimal and trivial :)

Wim Leers’s picture

Yay for #21!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Good stuff, thanks!

Committed and pushed to 8.x.

skyredwang’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

I am interested in porting #21 for D7, I was wondering if I should do it in a contributed module or directly patch against D7 core?

Wim Leers’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Fixed
Issue tags: -sprint, -Needs upgrade path

I very much doubt such a big change would be acceptable for D7 core. You'd have to do it in contrib.

Status: Fixed » Closed (fixed)

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

skyredwang’s picture

Taxonomy Changed Project is created for D7