EntityChangedTrait return type mismatch

Fix test assertions to expect integer rather than string.

Added draft change record: https://www.drupal.org/node/3262398

For developer sanity, entities should be patched to also return an integer for created time; adding a related issue where I do that for file entity.

Closed #2816829: Calling "getRevisionCreationTime" or "getCreatedTime" on entities returns string as a duplicate and adding credit.

One potential issue w/ this patch is that for some entity types, e.g. user and comment, the entity storage schema actually allows the changed field to be NULL.

So in the case of these nullable changed fields, the type hint for getChangedTime() and setChangedTime() is actually incorrect - it should be int|null. And we don't want to coerce a null changed time to zero.

Meanwhile for other entity types, the changed field is non-nullable; it must be an integer (via addSharedTableFieldIndex() with $not_null = TRUE).

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Reading comment #8

is there anyway to detect which entity types can't be nullable and maybe we can add some logic in?

@smustgrave whether or not the changed field can be null is typically defined by the entity's StorageSchema class.

But we don't really care whether or not it's allowed to be NULL. We can simply pass thru any NULL value, and otherwise coerce it to integer.

Ah gotcha thank you @mfb.

+++ b/core/lib/Drupal/Core/Entity/EntityChangedTrait.php
@@ -30,7 +30,8 @@ public function getChangedTimeAcrossTranslations() {
   public function getChangedTime() {
-    return $this->get('changed')->value;
+    $value = $this->get('changed')->value;
+    return isset($value) ? (int) $value : $value;

I checked and confirmed the docblock already specifies int. However, it looks like it returns int|null, correct? As discussed above.

Also, it'd be clearer to maybe have:

return isset($value) ? (int) $value : NULL;

I couldn't think of any legitimate usecases to rely on the timestamp being a string, but I think we should restrict the change to 10.1.x at least in case someone is doing something weird.

Also, what about the created time? Does it have the same issue? There's no trait for the created time, but it at least has the same issue of being documented as an int but possibly being null, and might have the same issue of being a string, since the code looks exactly the same for most implementations.

I checked and confirmed the docblock already specifies int. However, it looks like it returns int|null, correct?

Regarding the type hint, I wasn't clear on how best to handle this. Is changing it to int|null the consensus? As mentioned in the issue summary, for some entities the changed time is not allowed to be NULL (i.e. the database column does not allow NULL values; an exception is thrown if you try to save a NULL) while for other entities, the changed time can be NULL (and presumably actually is NULL in some cases, either because they were added before the changed field existed or were added programmatically and the changed field was missing). Given this discrepancy, two traits would be needed for correct type hints, one allowing NULL and one for the "NOT NULL" use case.

Also, what about the created time? Does it have the same issue?

I already created an issue to address File entity created and other type bugs: #3262358: Fix type hints in FileInterface to align with reality and as mentioned in the issue summary, we do need to work on all the other type bugs (which seems to be slowly happening thanks to usage of static analysis tools :)

I think if any implementation can return NULL, and the default implementation must handle a possible return value NULL, we must document that it is possible as a return value, even if some implementations never return NULL. It's a good question though! Any standardizing we do of the return types beyond that would need to happen with BC and deprecations and the like.

Ok tweaked this @return type doc. Technically getChangedTimeAcrossTranslations() could return NULL and setChangedTime() could accept NULL but considering those out of scope for the moment.

Looks like the change requested by @xjm in #15 was addressed.

Think this was an unrelated failure - back to RTBC

We need a re-roll here.

As patch #16 does not applies to drupal: 10.1.x so adding a reroll for this, please review.

#21 reverted some changes in HEAD

Thanks @mfb

@TanujJain-TJ if you had further thoughts can you add a comment/explanation?

It looks like #22 was deleted? So I'm re-uploading

@mfb I am sorry, did it by mistake, was suppose to change the status of another issue.

Committed 6bbb08f and pushed to 10.1.x. Thanks!

Sorry for posting in this old issue, but I was wondering: could it be that we forgot to update the return type of \Drupal\Core\Entity\EntityChangedInterface::getChangedTime() ?

This is still

/**
  * Gets the timestamp of the last entity change for the current translation.
  *
  * @return int
  *   The timestamp of the last entity save operation.
  */
public function getChangedTime();

While \Drupal\Core\Entity\EntityChangedTrait::getChangedTime() is now documented to be able to return NULL values:

  /**
   * Gets the timestamp of the last entity change for the current translation.
   *
   * @return int|null
   *   The timestamp of the last entity save operation. Some entities allow a
   *   NULL value indicating the changed time is unknown.
   */
  public function getChangedTime() {
    $value = $this->get('changed')->value;
    return isset($value) ? (int) $value : NULL;
  }

@rp7 Interesting, I guess phpstan doesn't warn about such issues in phpdoc? These need to be migrated to actual type declarations at some point..