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

Created on 21 September 2024, 3 months ago

Problem/Motivation

The method ContentEntityBase::getRevisionId() should not return string values. All entity revision id's are always of the integer type. According to the docblock the return type can be an integer or null or a string. MongoDB is much stricter with query conditions. For MongoDB a string with the character '4' is not the same as the integer 4. It would be very helpful for Drupal on MongoDB, when the method would only return an integer value or null when there is not revision id.

Proposed resolution

The docblock of the method ContentEntityBase::getRevisionId() will be changed and return values must be an integer or null. Add type casting to the return value of the method.

Remaining tasks

TBD

User interface changes

None

Introduced terminology

None

API changes

The method ContentEntityBase::getRevisionId() will no longer return string values.

Data model changes

None

Release notes snippet

TBD

πŸ“Œ Task
Status

Needs review

Version

11.0 πŸ”₯

Component
EntityΒ  β†’

Last updated about 8 hours ago

Created by

πŸ‡³πŸ‡±Netherlands daffie

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @daffie
  • Pipeline finished with Failed
    3 months ago
    Total: 934s
    #288920
  • Pipeline finished with Success
    3 months ago
    Total: 967s
    #288931
  • Status changed to Needs review 3 months ago
  • πŸ‡³πŸ‡±Netherlands daffie
  • πŸ‡³πŸ‡±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.

  • Pipeline finished with Failed
    3 months ago
    #292997
  • Pipeline finished with Failed
    3 months ago
    Total: 278s
    #293011
  • Pipeline finished with Failed
    3 months ago
    Total: 466s
    #295943
  • Pipeline finished with Canceled
    3 months ago
    Total: 230s
    #295961
  • Pipeline finished with Failed
    3 months ago
    Total: 426s
    #295964
  • Pipeline finished with Success
    3 months ago
    Total: 3089s
    #295987
  • πŸ‡³πŸ‡±Netherlands daffie

    The requested deprecation has been added.

  • πŸ‡©πŸ‡ͺ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.

  • Pipeline finished with Success
    about 2 months ago
    Total: 474s
    #317875
  • πŸ‡³πŸ‡±Netherlands daffie

    Rebased and back to RTBC.

  • πŸ‡·πŸ‡΄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.

Production build 0.71.5 2024