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

Can we get the change record updated here to reflect that this isn't just for comments?

Because this has the potential to break contrib tests (as seen by the test changes needed for core) I feel like this is too disruptive for a patch release and would need to be minor release only

Thanks for working on this 🎉

Updated the change record

Ready for review - this removes some potential edge case behavior changes where null would have been cast to 0.

But, the methods are not documented to return null. So, I think either a deprecation should be triggered if null is returned, or the documentation could be changed to allow null, despite the data model not allowing null.

Shouldn't the todos point to an issue

Can we please update the todos to the issue they’re suppose to be solved in please

Is there an existing issue about those?

We didn't make another issue AFAIK. The current state of the patch is "for review" to figure out next steps.

I have created 🐛 ::getCreatedTime() sometimes returns null instead of integer Active .

@liam morland did you see my comments in #30? I feel like it would be helpful if this issue could decide to either trigger a deprecation if null is returned in cases where it is not documented to be possible, or change the documentation to allow null, despite the data model not allowing null.

In other words, my @todo was something to figure out in this issue, not a future issue.

I have had a look at the CR and the MR and recent comments. I note there are 2x todos, 1 in User.php and 1 in Workspace.php. It would seem a follow-up issue could deal with those? Since the 2x timestamps involved in the todos are unusual and need to be handled differently since we are allowing safe handling of NULL values for timestamp in this issue but these 2x todos are when NULL is not currently supposed to be allowed.

Worth taking a look at the phpdocs for the 2x timestamps to see why they should not return NULL and if should throw an exception or how null values are to be dealt with.

@#37: The current issue is pretty straight-forward: Return int instead of string. Deciding what to do about the nulls is a bigger topic. So, I think it makes sense to surface that issue and put it into its own issue.

#39 +1. Are we ready for RTBTC? We have test coverage in place, CR seems done.. Pipeline green?

oily → changed the visibility of the branch 11.x to hidden.