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

Files: 
CommentFileSizeAuthor
#21 2074229-21-term-changed.patch5.99 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,344 pass(es).
[ View ]
#21 interdiff.txt818 bytesGábor Hojtsy
#19 interdiff.txt2.2 KBGábor Hojtsy
#19 2074229-19-term-changed.patch5.88 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,491 pass(es).
[ View ]
#16 2074229-16-term-changed_no-update.patch3.58 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 58,380 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
#16 2074229-16-term-changed.patch4.01 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,490 pass(es).
[ View ]
#16 2074229-interdiff-9-16.txt819 byteststoeckler
#9 term-changed-timestamp-9.patch2.89 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,802 pass(es).
[ View ]
#3 term-changed-timestamp-3.patch2.47 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,248 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
term-changed-timestamp.patch2.56 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,653 pass(es), 4 fail(s), and 234 exception(s).
[ View ]

Comments

Issue tags:+Needs upgrade path

Also needs upgrade path.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.47 KB
FAILED: [[SimpleTest]]: [MySQL] 58,248 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

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?

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new2.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,802 pass(es).
[ View ]

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

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.

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.

Issue tags:+sprint, +Spark

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

Yeah, OK, I guess.

The patch itself looks but needs upgrade path tests.

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

Totally did not want to remove the tag.

Status:Needs work» Needs review
Issue tags:-Needs upgrade path tests
StatusFileSize
new819 bytes
new4.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,490 pass(es).
[ View ]
new3.58 KB
FAILED: [[SimpleTest]]: [MySQL] 58,380 pass(es), 4 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Looks good to me :)

What is the chance of porting this patch to D7?

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new5.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,491 pass(es).
[ View ]
new2.2 KB

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

Status:Needs review» Reviewed & tested by the community

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

StatusFileSize
new818 bytes
new5.99 KB
PASSED: [[SimpleTest]]: [MySQL] 58,344 pass(es).
[ View ]

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

Yay for #21!

Status:Reviewed & tested by the community» Fixed

Good stuff, thanks!

Committed and pushed to 8.x.

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?

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.

Taxonomy Changed Project is created for D7