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

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

Postponed on 📌 Deprecate NodeStorage::updateType and remove type change handling in NodeType::postSave Active

📌 Deprecate NodeStorage::updateType and remove type change handling in NodeType::postSave Active has landed.

@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

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

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?

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 :)

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

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.

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

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.

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.

> 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.

Would be good to get another set of eyes on this.

I've removed the interface now so that's taken care of

Last decision needed for https://git.drupalcode.org/project/drupal/-/merge_requests/11828#note_50...

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.