- 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 ActiveAs for the remaining:
-::revisionIds
is used in a handful of tests and inNodeRevisionDeleteForm
-::userRevisionIds
is used in a handful of hooks insideNodeHooks
andNodeHooks1
(these classes will change in π Remove NodeHooks1 Active
-::clearRevisionsLanguage
is used by a single hook inNodeHooks1::configurableLanguageDelete
- π¦πΊAustralia acbramley
Postponed on π Deprecate NodeStorage::updateType and remove type change handling in NodeType::postSave Active
- π³π±Netherlands daffie
π Deprecate NodeStorage::updateType and remove type change handling in NodeType::postSave Active has landed.
- π¦πΊ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
- π³π±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.
- πΊπΈ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.