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

  1. Views support — similar to #2074229: Add changed time tracking to taxonomy terms.
  2. 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.

Contributor tasks needed
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
Files: 
CommentFileSizeAuthor
#52 2074255-52.patch7.75 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,751 pass(es). View
#52 interdiff-50-52.txt636 bytescilefen
#50 2074255-50.patch7.75 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,560 pass(es). View
#43 2074255-43.patch7.73 KBamunir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2074255-43.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

Gábor Hojtsy’s picture

Issue tags: +Needs upgrade path

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

function user_login_finalize(UserInterface $account) {
  // ...
  $account->setLastLoginTime(REQUEST_TIME);
  db_update('users')
    ->fields(array('login' => $user->getLastLoginTime()))
    ->condition('uid', $user->id())
    ->execute();
  // ...
}

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.

Gábor Hojtsy’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

Wim Leers’s picture

Issue tags: -sprint

We're not working on this right now.

Wim Leers’s picture

Status: Needs work » Needs review

user-changed-timestamp.patch queued for re-testing.

Status: Needs review » Needs work

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

Wim Leers’s picture

Issue summary: View changes
Issue tags: +Novice
Jalandhar’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,656 pass(es), 2 fail(s), and 2,951 exception(s). View

Updating the rerolled patch.

Please review.

Status: Needs review » Needs work

The last submitted patch, 8: drupal8-user_changed_timestamp-2074255-8.patch, failed testing.

JeroenT’s picture

Assigned: Unassigned » JeroenT
JeroenT’s picture

Assigned: JeroenT » Unassigned
Status: Needs work » Needs review
FileSize
2.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,685 pass(es). View

This patch fixes the changed time tracking.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs upgrade path

#11: Thanks! Please provide an interdiff next time though :)

  1. +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
    @@ -114,6 +114,9 @@ public function preSave(EntityStorageInterface $storage) {
    +
    +    // Before saving the user, set changed time.
    +    $this->changed->value = REQUEST_TIME;
    

    This is no longer necessary. You call FieldDefinition::create('changed'), which creates a field of the type changed. If you look at the corresponding ChangedItem (introduced in 533ee3a17f7ee2aa310ea1a1146a13e6c8201482
    http://drupalcode.org/project/drupal.git/commitdiff/533ee3a17f7ee2aa310ea1a1146a13e6c8201482">533ee3a17f7ee2aa310ea1a1146a13e6c8201482
    ), you'll see that this is already done automatically there for you :)

  2. +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
    @@ -529,6 +539,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      $fields['changed'] = FieldDefinition::create('changed')
    +          ->setLabel(t('Changed'))
    +          ->setDescription(t('The time the user was most recently saved.'));
    

    The first line is indented too much by two spaces, the second level by four.

  3. +++ b/core/modules/user/lib/Drupal/user/UserInterface.php
    @@ -179,4 +179,11 @@ public function block();
    +  /**
    +   * Returns the user modification timestamp.
    +   *
    +   * @return int
    +   *   User modification timestamp.
    +   */
    +  public function getChangedTime();
    

    This should go away, UserInterface should simply extend EntityChangedInterface.

    Look at NodeInterface for an example :)

Wim Leers’s picture

Issue summary: View changes

I just realized Views support is also necessary, but you can find a great example at #2074229: Add changed time tracking to taxonomy terms :)

wiifm’s picture

Status: Needs work » Needs review
Issue tags: +#Drupal8nz
FileSize
4.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
4.59 KB

Re-factored the patch based on review by @Wim Leers.

Status: Needs review » Needs work

The last submitted patch, 16: 2074255-16.patch, failed testing.

wiifm’s picture

Status: Needs work » Needs review
FileSize
4.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2074255-18.patch. Unable to apply patch. See the log in the details link for more information. View
646 bytes

Now with 100% more required methods, hopefully this pleases the testbot.

wiifm’s picture

Issue summary: View changes
cilefen’s picture

18: 2074255-18.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 18: 2074255-18.patch, failed testing.

cilefen’s picture

tstoeckler’s picture

+++ b/core/modules/user/user.install
@@ -123,6 +123,12 @@ function user_schema() {
+      'changed' => array(
+        'description' => 'The Unix timestamp when the user was most recently saved.',
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
+      ),

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

vegantriathlete’s picture

Status: Needs work » Needs review
FileSize
5.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2074255-24.patch. Unable to apply patch. See the log in the details link for more information. View
cilefen’s picture

Status: Needs review » Needs work
Issue tags: -Spark, -#Drupal8nz +Needs tests

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

+++ b/core/modules/user/src/Entity/User.php
@@ -528,6 +535,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['changed'] = FieldDefinition::create('changed')
+      ->setLabel(t('Changed'))
+      ->setDescription(t('The time that the user was last edited.'));
+

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.

AndyThornton’s picture

18: 2074255-18.patch queued for re-testing.

The last submitted patch, 18: 2074255-18.patch, failed testing.

almul0’s picture

FileSize
1.25 KB
4.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,472 pass(es). View

Rerolled patch as it failed to apply.
Moved up the field changed on entity definition.

Still left to write the tests.

almul0’s picture

almul0 queued 24: 2074255-24.patch for re-testing.

The last submitted patch, 24: 2074255-24.patch, failed testing.

cilefen’s picture

FileSize
463 bytes
4.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,255 pass(es). View

Fixes a whitespace error and checking the test status.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
5.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,182 pass(es). View

Tests the "changed" and "created" timestamps. I'd like to also add a views test before this is committed.

cilefen’s picture

Issue summary: View changes
cilefen’s picture

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

cilefen’s picture

Issue summary: View changes
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me. Added a "updated date" column to the default user_admin_people view in Standard. Works fine. Analogous to Term and taxonomy.views.inc.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

cilefen’s picture

Status: Needs work » Needs review
FileSize
2.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,040 pass(es), 2 fail(s), and 1 exception(s). View
7.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,105 pass(es). View

Adds a views tests.

cilefen’s picture

FileSize
2.14 KB

The last submitted patch, 39: interdiff-33-39.patch, failed testing.

amunir’s picture

Assigned: Unassigned » amunir

Rerolling!

amunir’s picture

Assigned: amunir » Unassigned
FileSize
7.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2074255-43.patch. Unable to apply patch. See the log in the details link for more information. View

rerolled path 39 patch.

mparker17’s picture

As of #43, there is a "UserCreateTest", "UserEditTest", "UserChangedTest", and a test view. Is the "Needs tests" tag still applicable?

cilefen’s picture

Issue tags: -Needs tests

Gábor Hojtsy queued 43: 2074255-43.patch for re-testing.

Gábor Hojtsy’s picture

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

+++ b/core/modules/user/src/Entity/User.php
@@ -500,6 +507,10 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['changed'] = FieldDefinition::create('changed')
+    ->setLabel(t('Changed'))
+    ->setDescription(t('The time that the user was last edited.'));

Should indent the second and third lines.

Otherwise looks great.

Status: Needs review » Needs work

The last submitted patch, 43: 2074255-43.patch, failed testing.

cilefen’s picture

Issue tags: +Needs reroll
cilefen’s picture

Status: Needs work » Needs review
FileSize
7.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,560 pass(es). View

Reroll of FieldDefinition to BaseFieldDefinition.

cilefen’s picture

Issue tags: -Needs reroll
cilefen’s picture

FileSize
636 bytes
7.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,751 pass(es). View

Oh, here are the indents.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks all good :) Thanks!

Gábor Hojtsy’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2acb1fd and pushed to 8.0.x. Thanks!

  • alexpott committed 2acb1fd on 8.0.x
    Issue #2074255 by cilefen, wiifm, almul0, amunir, Gábor Hojtsy,...

Status: Fixed » Closed (fixed)

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