Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
user.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
26 Aug 2013 at 13:50 UTC
Updated:
26 Aug 2014 at 17:40 UTC
Jump to comment: Most recent, Most recent file
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 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,
UserInterfaceshould simply extendEntityChangedInterface.Look at
NodeInterfacefor 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 commented18: 2074255-18.patch queued for re-testing.
Comment #22
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 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 commented18: 2074255-18.patch queued for re-testing.
Comment #28
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 commentedComment #32
cilefen commentedFixes a whitespace error and checking the test status.
Comment #33
cilefen commentedTests the "changed" and "created" timestamps. I'd like to also add a views test before this is committed.
Comment #34
cilefen commentedComment #35
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 commentedComment #37
wim leersLooks great to me. Added a "updated date" column to the default
user_admin_peopleview in Standard. Works fine. Analogous toTermandtaxonomy.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 commentedAdds a views tests.
Comment #40
cilefen commentedComment #42
amunir commentedRerolling!
Comment #43
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 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 commentedComment #50
cilefen commentedReroll of FieldDefinition to BaseFieldDefinition.
Comment #51
cilefen commentedComment #52
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!