getCreatedTime returns string instead of integer

Updated the logic to correctly return an integer.

nils.destoop → changed the visibility of the branch 3453210-getcreatedtime-returns-string to hidden.

Think maybe the docs should be updated? You can see the change causes test failures so there is backwards compatibility concerns with making this change.

The docs that need to be updated is the quick fix, but imo a dirty fix. It makes no sense to return an int for getChangedTime and a string for getCreatedTime.
I agree that there could. be backwards compatibility issues, but a simple release note can warn users for this.

The failing test that I just fixed also shows how illogical it is:

before:

    $this->assertSame('1421727536', $comment->getCreatedTime());
    $this->assertSame(1421727536, $comment->getChangedTime());

after:

    $this->assertSame(1421727536, $comment->getCreatedTime());
    $this->assertSame(1421727536, $comment->getChangedTime());

Right but about the BC concern this could immediately start breaking someone's code with no warning.

Discussed in slack with @catch, for reference https://drupal.slack.com/archives/C04CHUX484T/p1718289507191399

Created a CR just in case.

But does have a legit failure.

Setting parent issue to 🌱 [META] Stronger Typing for Entity API Active .

liam morland → made their first commit to this issue’s fork.

Test failure fixed.

Looks good to me.

When we made this fix for File entity - https://www.drupal.org/node/3262371 → - I assumed that adding type declarations would need to happen in a major version, so I didn't work on it in that issue.

Tagging "needs followup" since there are other cases of getCreatedTime() and getRevisionCreationTime() that need to be fixed

By the way, for comment entities, created time cannot *really* be null, as that column has a NOT NULL specification, so you cannot save a comment with a null created time; when you create a new comment entity, the created time is automatically initialized to the current time. So I'm not sure why or in what circumstances this method would return null, aside from someone forcibly setting the created time to null..

I grepped Drupal core for "function getCreatedTime" and fixed all of them. All except File are documented in their interfaces to return int, so I have it doing that.

I did not add any type declarations.

@liam morland File entity allows NULL created time, and thus databases may actually contain NULL for that column. That's why I changed FileInterface to return int|null.

I believe most entities have a NOT NULL created column, but it's worth double checking them.

OK, in every other case, the interface already said that only int is allowed.

@liam morland I was referring to how we need to look at the data model. For example, Media and Workspace entity, like File entity, allow a NULL created time to be stored in the database. So we need to allow the method to return NULL in these cases, and those interfaces need to be adjusted to match.

It's a pain but will need to add some backwards coverage. 🐛 The method ContentEntityBase::getLoadedRevisionId() should not return string values Needs review is a good example.

• Also fix RevisionLogEntityTrait::getRevisionCreationTime() to return integers, as it is documented to
• Adjust MediaInterface::getCreatedTime, NodeInterface::getRevisionCreationTime() and RevisionLogInterface::getRevisionCreationTime() to allow NULL return value, as the data model allows NULL values

Believe #18 still holds true. If someone is checking if this is a string it’ll immediately break. So still think some BC coverage is needed

@smustgrave Can you explain what you mean by BC coverage?

I believe all we need here is a change record (and some reviews to sanity check this :)

This is a bugfix because these methods are already documented to return an integer.

This is analogous to similar bug fixes I made for changed time - 🐛 EntityChangedTrait return type mismatch RTBC - and File created time - 🐛 Fix type hints in FileInterface to align with reality Fixed - where all we needed were change records.

If you look at the issue in #18 essentially a deprecation had to be added to return a string

@smustgrave is there a scenario where a deprecation would be needed here? We didn't need a deprecation for EntityChangedTrait or File created time.

May be low. But you had to fix tests to match this change. Possible someone was doing the same on their contrib?

This is why a change record is needed. Basically the same test changes were made when we fixed changed time - now these two fields will finally match.

I’ll leave in review but think this could be a breaking change