Create a new NodeRepository for the code from Drupal\node\NodeStorage

Created on 23 October 2023, over 1 year ago

Problem/Motivation

The database driver for MongoDB needs to override the class Drupal\Core\Entity\Sql\SqlContentEntityStorage. MongoDB does document/JSON storage instead of relational database storage.
The Node entity is overriding that class in Drupal\node\NodeStorage. The problem for the database driver for MongoDB is that it then also needs to override the methods in Drupal\node\NodeStorage. It also needs to duplicate the code inside Drupal\file\FileStorage too. Duplicating code is wrong.
Another problem is that the code in Drupal\node\NodeStorage class is that it is NOT extending the functionality from the class Drupal\Core\Entity\Sql\SqlContentEntityStorage. The extra code can easily being moved to another class.

Proposed resolution

- Create a new Drupal\node\NodeRepository class/service and move the code/methods from Drupal\node\NodeStorage to the new class/service.
- Deprecate the methods and the class Drupal\node\NodeStorage.

Remaining tasks

User interface changes

None

API changes

- New service node_repository with the methods revisionIds(), userRevisionIds(), countDefaultLanguageRevisions(), updateType() and clearRevisionsLanguage().
- The methods revisionIds(), userRevisionIds(), countDefaultLanguageRevisions(), updateType() and clearRevisionsLanguage() in the class Drupal\node\NodeStorage are deprecated.
- The class Drupal\node\NodeStorage is deprecated.

Data model changes

None

Release notes snippet

None

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component
Node systemΒ  β†’

Last updated about 5 hours ago

No maintainer
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
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Definitely agree with this one! I never knew NodeStorage only implemented its own special functions.

    There are 2 functions that I'd get rid of (deprecate first of course) entirely:
    - ::countDefaultLanguageRevisions - this isn't used anywhere in the core codebase
    - ::updateType - this function is just scary, see πŸ“Œ Deprecate NodeStorage::updateType and remove type change handling in NodeType::postSave Active

    As for the remaining:
    - ::revisionIds is used in a handful of tests and in NodeRevisionDeleteForm
    - ::userRevisionIds is used in a handful of hooks inside NodeHooks and NodeHooks1 (these classes will change in πŸ“Œ Remove NodeHooks1 Active
    - ::clearRevisionsLanguage is used by a single hook in NodeHooks1::configurableLanguageDelete

  • πŸ‡¦πŸ‡ΊAustralia acbramley
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    @berdir mentioned in slack that some of these methods could actually be deprecated without replacement so this will need a bit more investigation to see if there are existing methods that could replace them.

    Quoting @berdir:

    for example revisionIds(), the new generic revision controller doesn't use that, it uses an entity query afaik

  • Merge request !11828Issue #3396062: Deprecate NodeStorage β†’ (Open) created by acbramley
  • Pipeline finished with Failed
    10 days ago
    Total: 139s
    #473516
  • Pipeline finished with Failed
    10 days ago
    Total: 121s
    #473525
  • Pipeline finished with Failed
    10 days ago
    #473529
  • Pipeline finished with Failed
    10 days ago
    Total: 133s
    #473532
  • πŸ‡¦πŸ‡ΊAustralia acbramley
  • Pipeline finished with Success
    10 days ago
    Total: 2067s
    #473533
  • πŸ‡¦πŸ‡ΊAustralia acbramley
  • πŸ‡³πŸ‡±Netherlands daffie

    It all looks good to me.
    The only thing I am missing is the deprecation message testing.
    Everything else is RTBC for me.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    The only thing I am missing is the deprecation message testing.

    I've not seen that done recently on really any issues that deprecate anything, do we still need them?

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Quoting @catch from slack as I wanted clarity on when we need deprecation tests:

    If it's only for code that's going to be removed, then we're explicitly avoiding deprecation tests because it mostly adds to code churn - no point testing a one line call to @trigger_error()
    If there is a tricky bc layer for something, testing of the bc layer is good.
    From a quick look at the MR, this one is in the first category, so I think nothing needed.

    Moving back to NR :)

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Reviewed. We can not remove the storage handler here, that will break all existing usages.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Added revisionIds to the new service and tidied up the (internal) form a bit. Also removed the post update as we're not changing the handler anymore.

  • Pipeline finished with Success
    8 days ago
    Total: 552s
    #475465
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    With regards to clearRevisionsLanguage() know it was used in 1 spot but could it be used in contrib?

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Left one comment about one of the removed methods.

    > The service will also be used for https://www.drupal.org/project/drupal/issues/2004368 πŸ“Œ Introduce node_mass_delete() or make node_mass_update() more flexible Active

    The problem is that as soon as this is in a stable release with an interface, it's really hard to extend it.

    If it just does entity queries, then one option IMHO is to not add an interface because there isn't really a reason to provide a completely different implementation.

    But I'm also not really convinced that having a NodeRepository for those method is really useful. The referenced issue could also add its own service/class, that's definitely easier to deal with for BC.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Re #17

    If it just does entity queries, then one option IMHO is to not add an interface because there isn't really a reason to provide a completely different implementation.

    Happy to remove the interface, I agree but this was just following patterns already in core where it seems like every service has an interface that could often fall into the same bucket of not really making sense.

    The referenced issue could also add its own service/class, that's definitely easier to deal with for BC.

    BC for what? I'm a bit confused why adding new methods to a service would be hard for BC.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    > BC for what? I'm a bit confused why adding new methods to a service would be hard for BC.

    Adding new methods to the *interface*.

    If we actually want to support different implementations then those would break if we add new methods to NodeRepositoryInterface. We have the 1:1 basea class/interface rule but that's mostly for base classes, wouldn't really apply here.

    And yes, especially in the past we always added interfaces for everything, but I think it's a pattern we're slowly moving away from.

Production build 0.71.5 2024