- Issue created by @daffie
- Merge request !9563Changed the method ContentEntityBase::getRevisionId() to only return an integer value or null β (Open) created by daffie
- Status changed to Needs review
3 months ago 9:55am 21 September 2024 - π³π±Netherlands daffie
@smustgrave: I have thought about any possible BC problems and I do not see them. I could be wrong. If you see any, than please explain.
- πΊπΈUnited States smustgrave
From the slack conversation, but sounds like we will have to trigger a deprecation for the string.
- π©πͺGermany tobiasb Berlin
I do not like the way. I mean you can just add cast by yourself. there is no win/improvement without do the cast by default.
$id = (int) $entity->getRevisionId(); // NULL -> 0 if ($id) { // Do something. }
Cast by default:
$id = $entity->getRevisionId(); // NULL || >= 1 if ($id) { // Do something. }
I would just do the cast by default in 12.x.
- π³π±Netherlands daffie
@tobiasb: Thank you for your concern. This issue is a preparation issue for getting Drupal to work with MongoDB. Drupal on MongoDB is needed for creating Drupal sites with a huge number of authenticated users. Adding a type hint with every call to this method will result that you as a programmer will sometimes forget to add the type hint. MongoDB is very strict in variables being of the correct type. With a relational database like MySQL, MariaDB and PostgreSQL they are very forgiving. Fro them there is no difference between the string '4' and the integer 4. For MongoDB they are totally 2 different things. The added parameter is temporary and will be removed in Drupal 12.0. We do not want to break peoples existing sites, therefore we are adding the deprecation in that way.
- πΊπΈUnited States smustgrave
Reviewed the change and backwards compatibility appears to be addressed. Going to be fun removing those TRUE statement in D12 haha but looks good!
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.
- π·π΄Romania amateescu
I think we should wait for the outcome of the discussion in π The method ContentEntityBase::getLoadedRevisionId() should not return string values Needs review since they're very much related.
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.