Problem/Motivation

Entities can have string or int entity IDs. String IDs may not necessarily be numeric or >=0.

Proposed resolution

Update param doc to accept string|int

Remaining tasks

Update docs.

User interface changes

Nil

API changes

Nil

Data model changes

Nil

Release notes snippet

Trivial, not required.

CommentFileSizeAuthor
#3 3390366-3.patch617 bytessoham sengupta

Comments

dpi created an issue. See original summary.

dpi’s picture

dpi
Location Perth, Australia
commented

It would be easy to copy the param doc from \Drupal\Core\Entity\EntityStorageInterface::loadRevision from the Drupal 10 branches (it is deprecated for D11)

soham sengupta’s picture

soham sengupta commented
Status: Active » Needs review
StatusFileSize
new 3390366-3.patch617 bytes

Hi, I have updated the param doc. Please review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Param doc was copied from \Drupal\Core\Entity\EntityStorageInterface::loadRevision

dpi’s picture

dpi
Location Perth, Australia
commented

LGTM

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -Novice

I think this issue should go a bit further and make consistent.

\Drupal\Core\Entity\EntityRepository::loadRevision()
\Drupal\Core\Entity\RevisionableInterface::getLoadedRevisionId()
\Drupal\Core\Entity\RevisionableStorageInterface::deleteRevision()

I also think it is worth asking is it really true that revision IDs can be non numeric strings. Yes entity IDs can be strings but that is not the same as a revision ID. I think a revision ID should always be an integer - I'd be amazed if there is any true string revision IDs in contrib or custom. So maybe the fix here is to change them to int and add some casting.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.