getCreatedTime returns string instead of integer

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.

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 #3441689: [META] Stronger Typing for Entity API.

Test failure fixed.

Would it be OK to add a return type declaration?

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.

@mfb should FileInterface be set back to how it used to be, documenting that it will always return int?

The change record will need an update to match the changes I just made. If it is agreed to go wit these changes, let me know and I will update the change record.

@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. #3475921: The method ContentEntityBase::getLoadedRevisionId() should the return value with the correct type 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 - #3210064: EntityChangedTrait return type mismatch - and File created time - #3262358: Fix type hints in FileInterface to align with reality - 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 #3533981: ::getCreatedTime() sometimes returns null instead of integer and added @see comments to the @todo comments.

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

I think we either leave the todo's in place and add info if necessary and create a follow-up for them

or

Fix them in this issue if it is a simple fix.

It looks to me like #28 has been done. The CR looks okay.

@#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? Someone can create a follow-up. Not sure we need to wait for that to happen?

I applied 2 changes to the @todo comments and updated credit.

The change record is correct although it needs version number updates.

All the feedback going back to @larowlan's review has been addressed. The change record has been updated to mention this impacts entity types other than the Comment module. The @todo comments link to the follow-up issue. I also read through the changes and don't have any additional feedback. So I'm going to RTBC it.

Wouldn't it be better to do #2833378: Create an EntityCreatedInterface with accompanying trait to promote code reuse and standardization instead?

That could happen eventually. This issue is an easier fix that is ready to go.

MR has a conflict.