The method ContentEntityBase::getLoadedRevisionId() should not return string values

As mentioned in slack will have to do some kind of deprecation.

The requested deprecation has been added.

Thanks for adding that BC layer. Will be fun to remove those TRUEs later but seems like correct path.

I don't think we can make this change.
I have client projects that import content from external systems where the revision ID is a UUID and therefore is a string.

There's nothing stopping a content entity's baseFieldDefinitions from defining the revision ID as a string.

I genuinely think this is `Closed (won't fix)` in lieu of `Closed (can't fix)`

Hi @larowlan,

Using revision ids that are not an integer value is not supported by Drupal Core. The docblock of RevisionableInterface::getLoadedRevisionId() is clearly stating that the method only returns an integer value. What your client project is doing is NOT supported by Drupal core.
The problem for me with the database driver for MongoDB is that MongoDB in conditions is doing === and relational databases do ==. For them the condition revision_id = '4' is not a problem. For MongoDB it will always be FALSE.
Would it be acceptable to only return an integer value when ctype_digit($this->loadedRevisionId) is TRUE? This would work for MongoDB and it would work non-integer revision ids.

Yeah that sounds good
We can also trigger a deprecation if it's not so sites like this client project get notification that they're doing it wrong

As discused with @larowlan: the loaded revision ID will only return an integer value when all characters of the loaded revision ID are numeric. The CR is updated for this change.

Feedback appears to be addressed for this one.

There are a few problems here:

1. why should we treat getLoadedRevisionId() differently than getRevisionId()?
2. the new argument defaults to FALSE but we update every usage of that method to pass TRUE. Wouldn't it be easier to not introduce the argument in the first place and only trigger the deprecation when !ctype_digit($this->loadedRevisionId)?

Using revision ids that are not an integer value is not supported by Drupal Core. The docblock of RevisionableInterface::getLoadedRevisionId() is clearly stating that the method only returns an integer value. What your client project is doing is NOT supported by Drupal core.

That's not actually true, the @return doxygen for \Drupal\Core\Entity\RevisionableInterface::getRevisionId() is int|null|string.

There is indeed a somewhat "implied" requirement that revision IDs are sequential integers (mostly with sort queries), but it's not strictly enforced. And UUIDv7 strings would also satisfy that requirement btw :)

why should we treat getLoadedRevisionId() differently than getRevisionId()?

I do not want to treat them diferently. I also have 📌 The method ContentEntityBase::getRevisionId() should not return string values Needs review for the other method. The solution from this issue should also be used in the other issue. I have learned that smaller issues are easier to lands then bigger issues. That is why I split them up.

the new argument defaults to FALSE but we update every usage of that method to pass TRUE. Wouldn't it be easier to not introduce the argument in the first place and only trigger the deprecation when !ctype_digit($this->loadedRevisionId)?

Yes, that would work for triggering the deprecation. Only that does not allow us to change the type of the return value in core without doing the same in contrib/custom code. Doing it in contrib/custom code might cause a BR break and that is not allowed.

That's not actually true, the @return doxygen for \Drupal\Core\Entity\RevisionableInterface::getRevisionId() is int|null|string.

That change has been made 2 year ago ( #2941148: Fix Drupal.Commenting.FunctionComment.MissingReturnType → ) to fix a PHPCS rule. In Drupal Core are all used revision IDs integers. No string revisions IDs. Maybe a good idea for a followup.

I am not against supporting UUIDs as revision IDs. What I would like for MongoDB is that revision IDs are returned as the correct type. When they are an integer they should be returned as an integer value, not a string value. When they are a string value like with a UUID then they should be returned as a string value. I have updated the title and the IS.

What I would like for MongoDB is that revision IDs are returned as the correct type. When they are an integer they should be returned as an integer value, not a string value.

The problem is (and this applies to 📌 The method ContentEntityBase::getRevisionId() should not return string values Needs review as well) that it's not currently possible within the Entity API to type cast anything. And that's because $entity->get('revision_id')->value returns a string, and we can't really enforce a policy for not using that way of retrieving the revision ID. So even we change getLoadedRevisionId() and getRevisionId() to return integers, the MongoDB driver has no actual guarantee that's what it will receive in all cases.

I think a better way would be for MongoDB to typecast field values by itself when needed, but I don't know the codebase at all to make an actual informed proposal for where it should do that.

The problem is that it's not currently possible within the Entity API to type cast anything.

I think we can get the type casting from the field storage type. We are doing that already in the function _comment_entity_uses_integer_id() and in the method Drupal\Core\Entity\ContentEntityStorageBase::cleanIds(). I have updated the PR to use the field storage type for type casting the return value.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.