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