Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Users 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 users.
Remaining tasks
Views support — similar to #2074229: Add changed time tracking to taxonomy terms.- Get to RTBC! Reviewers, if manual testing, you will need to be sure the changed column is created and updated with a timestamp when a user is updated or created. Also, be sure you can display the "Updated date" column for users in views.
User interface changes
None.
API changes
Schema changed. New User::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.
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Reroll the patch if it no longer applies. | Instructions | yes | |
Add automated tests | Instructions | yes | |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
Comment | File | Size | Author |
---|---|---|---|
#52 | 2074255-52.patch | 7.75 KB | cilefen |
#52 | interdiff-50-52.txt | 636 bytes | cilefen |
#50 | 2074255-50.patch | 7.75 KB | cilefen |
#43 | 2074255-43.patch | 7.73 KB | amunir |
Comments
Comment #1
Gábor HojtsyNeeds upgrade path.
Also checked if this would work with the login timestamp tracking and it seems to be fine. This code is used to update the login timestamp:
So saving the login timestamp is not a full user save operation. Therefore by putting the update of timestamp to the preSave() this should in fact apply only when actual user data is saved.
Comment #2
Gábor HojtsyComment #4
Wim LeersWe're not working on this right now.
Comment #5
Wim Leersuser-changed-timestamp.patch queued for re-testing.
Comment #7
Wim LeersComment #8
Jalandhar CreditAttribution: Jalandhar commentedUpdating the rerolled patch.
Please review.
Comment #10
JeroenTComment #11
JeroenTThis patch fixes the changed time tracking.
Comment #12
Wim Leers#11: Thanks! Please provide an interdiff next time though :)
This is no longer necessary. You call
FieldDefinition::create('changed')
, which creates a field of the typechanged
. If you look at the correspondingChangedItem
(introduced in 533ee3a17f7ee2aa310ea1a1146a13e6c8201482http://drupalcode.org/project/drupal.git/commitdiff/533ee3a17f7ee2aa310ea1a1146a13e6c8201482">
533ee3a17f7ee2aa310ea1a1146a13e6c8201482
), you'll see that this is already done automatically there for you :)The first line is indented too much by two spaces, the second level by four.
This should go away,
UserInterface
should simply extendEntityChangedInterface
.Look at
NodeInterface
for an example :)Comment #13
Wim LeersComment #14
Wim LeersComment #15
Wim LeersI just realized Views support is also necessary, but you can find a great example at #2074229: Add changed time tracking to taxonomy terms :)
Comment #16
wiifmRe-factored the patch based on review by @Wim Leers.
Comment #18
wiifmNow with 100% more required methods, hopefully this pleases the testbot.
Comment #19
wiifmComment #20
cilefen CreditAttribution: cilefen commented18: 2074255-18.patch queued for re-testing.
Comment #22
cilefen CreditAttribution: cilefen commentedUpdating for Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead...
Comment #23
tstoecklerThis hunk can be removed now, due to #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities.
On an unrelated note: see also #2209971: Automatically provide a changed field definition.
Comment #24
vegantriathleteComment #25
cilefen CreditAttribution: cilefen commentedI am manually testing this patch and I noticed a few things:
The changeddatabase column is created and is in fact updated when a user is saved, so that is good.
The changed field definition should be moved up to be right after the created field to match the pattern in the node entity.
The next task is to write tests.
Comment #26
AndyThornton CreditAttribution: AndyThornton commented18: 2074255-18.patch queued for re-testing.
Comment #28
almul0 CreditAttribution: almul0 commentedRerolled patch as it failed to apply.
Moved up the field changed on entity definition.
Still left to write the tests.
Comment #29
almul0 CreditAttribution: almul0 commentedComment #32
cilefen CreditAttribution: cilefen commentedFixes a whitespace error and checking the test status.
Comment #33
cilefen CreditAttribution: cilefen commentedTests the "changed" and "created" timestamps. I'd like to also add a views test before this is committed.
Comment #34
cilefen CreditAttribution: cilefen commentedComment #35
cilefen CreditAttribution: cilefen commented#2074229: Add changed time tracking to taxonomy terms, on which the views integration in this patch is based, doesn't add any additional views tests, so I'd say this is ready for final review.
Comment #36
cilefen CreditAttribution: cilefen commentedComment #37
Wim LeersLooks great to me. Added a "updated date" column to the default
user_admin_people
view in Standard. Works fine. Analogous toTerm
andtaxonomy.views.inc
.Comment #38
alexpott#35 Just because one patch does not add a test doesn't really mean that we should not here. I think we should at least have one test that shows that the changed field is available and usable by views.
Comment #39
cilefen CreditAttribution: cilefen commentedAdds a views tests.
Comment #40
cilefen CreditAttribution: cilefen commentedComment #42
amunir CreditAttribution: amunir commentedRerolling!
Comment #43
amunir CreditAttribution: amunir commentedrerolled path 39 patch.
Comment #44
mparker17As of #43, there is a "UserCreateTest", "UserEditTest", "UserChangedTest", and a test view. Is the "Needs tests" tag still applicable?
Comment #45
cilefen CreditAttribution: cilefen commentedComment #47
Gábor HojtsyWhile testing this through views looks like an overkill, it in fact tests the views integration as well as the feature. Which were my first thoughts. The only issue found in reviewing is:
Should indent the second and third lines.
Otherwise looks great.
Comment #49
cilefen CreditAttribution: cilefen commentedComment #50
cilefen CreditAttribution: cilefen commentedReroll of FieldDefinition to BaseFieldDefinition.
Comment #51
cilefen CreditAttribution: cilefen commentedComment #52
cilefen CreditAttribution: cilefen commentedOh, here are the indents.
Comment #53
Gábor HojtsyLooks all good :) Thanks!
Comment #54
Gábor Hojtsy@cilefen: btw #2074203: Add changed time tracking to menu links is outstanding of the parent meta and needs about the same thing. Would you be interested in looking at that one?
Comment #55
alexpottCommitted 2acb1fd and pushed to 8.0.x. Thanks!