Problem/Motivation
#2926540: Split revision-handling methods to a separate entity storage interface introduced a specialized interface for revisionable storage handlers and duplicated the loadRevision()
and deleteRevision()
methods from EntityStorageInterface
because those methods could not be deprecated during the 8.x.x cycle (see #2927251: [policy, no patch] Decide how to deal with methods needing to be moved down an interface hierarchy).
Proposed resolution
Remove those two methods in the last minor version of Drupal 8.
Remaining tasks
Wait for the 9.0.x branch to be created :)
User interface changes
Nope.
API changes
Revision-related methods will no longer be part of the generic EntityStorageInterface
.
Data model changes
Nope.
Comment | File | Size | Author |
---|
Issue fork drupal-2926958
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 2926958-remove-revision-related-methods changes, plain diff MR !3016
Comments
Comment #2
plachComment #3
Wim LeersWhy?
Comment #4
Wim LeersApparently the answer to my question is given in the first paragraph of the issue summary of the issue plach linked 😀👍
Comment #5
plachYep, sorry, updated the IS to reflect that.
Comment #7
andypostLet's see if something affected
Comment #8
andypostAlso removed unused implementations
Comment #11
andypostFix tests, not sure the change is right
maybe
\Drupal\Core\Entity\ContentEntityStorageInterface
is better choiceComment #12
andypostBit more fixes
Comment #14
andypostAs it pass now, needs review for test coverage changes mostly
Comment #15
BerdirThis looks fine to me. If we decide to remove these methods then this is does what needs to be done. The related policy issue was never resolved, but I'm not sure what exactly we could do more here. The only thing seems to be to add explicit @trigger_error() to the implementations that would go away in the future like ConfigEntityStorage, but there never were any reasons to call them in the first place, and it wouldn't have helped with the mocks in the tests that we need to fix here.
Comment #16
alexpottSo the interesting thing about this is that it messes with the typehints from things like this:
$entity = $this->entityTypeManager->getStorage('block_content')->loadRevision($this->configuration['block_revision_id']);
currently IDEs etc can resolve this to
\Drupal\Core\Entity\EntityStorageInterface::loadRevision
- with this change it can't be resolved.Comment #17
BerdirThat's a good point. we could document getStorage() that it may return any of the generic storage interfaces to improve that?
Comment #18
alexpottI think something needs to be done about #16 and #17 seems like the best solution.
Comment #19
xjmHmm, this slipped through the cracks; we should had made it a child of the deprecation removal meta.
I debated whether this should be moved to 9.1.x now. On the one hand, the docblock does tell them that it's deprecated, and removing a method from an interface is mostly BC aside from this edgecase:
On the other hand, though, that edgecase does exist, and the
@todo
doesn't tell the person in any detectable way.Could we try a more "proper" deprecation with something like what @plach suggested on the meta? That is:
Can we do that in 9.1, and then deprecate the methods for D10?
Comment #24
SpokjeCopying a few (relevant) comments over from #3213895: [META] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 10 branch:
@Spokje in #3213895-51:
@catch in #3213895-52:
Comment #25
SpokjeFirst stab at proper deprecation in 9.5.0.
Comment #26
SpokjeComment #27
SpokjeComment #28
longwaveI think each of the implementations should just say "There is no replacement." as it makes no sense to load revisions of config that is not revisionable in the first place, each of these methods just returned NULL.
To be clear the interface should still say "Use RevisionableStorageInterface instead" as that is the correct thing to do there.
Comment #29
Ratan Priya CreditAttribution: Ratan Priya at OpenSense Labs commentedComment #30
Ratan Priya CreditAttribution: Ratan Priya at OpenSense Labs commented@longwave,
I made the changes you required at comments #28
Needs review.
Comment #32
catchComment #33
MeenakshiG CreditAttribution: MeenakshiG at Srijan | A Material+ Company for Drupal India Association commentedUpdated the patch as mentioned in comments #28.
Added interdiff as well
Comment #34
SpokjeComment #35
SpokjeThere are a couple of references to
https://www.drupal.org/node/1
which seems to be a placeholder for a new Change Record.If that's the case, we need that CR to be created so we can link to it.
Comment #36
MeenakshiG CreditAttribution: MeenakshiG at Srijan | A Material+ Company for Drupal India Association commentedComment #37
BerdirI don't think this can still be deprecated for 10.0 which is soon in beta. It's too late and not important enough for an exception.
Move it to 10.1/11.0 and then is much more likely to land.
Comment #38
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI've made changes as per comment #33, please review. Still needs work for change record.
Comment #39
quietone CreditAttribution: quietone at PreviousNext commented@ravi.shankar, remember to check for other work that needs to be done. In #35 there is a request to update the link to the CR.
The draft change record for this issue was added in July 2022. Removing tag for a change record and adding the tag for change record updates.
This still needs work for #35 and the version in the issue meta data doesn't match the deprecation message.
Comment #41
SpokjeUpdated CR for 10.1.x
Comment #42
andypostnot sure removal of existing links is useful
Comment #43
SpokjeSwitch to MR workflow
Comment #45
andypostAdded feedback to MR, mostly nits
Comment #46
SpokjeThanks @andypost, resolved all your issues.
Comment #47
andypostThanks, should be ready
Comment #48
alexpottCommitted 3a9e58f and pushed to 10.1.x. Thanks!
Comment #49
andypostLooks not pushed