Problem/Motivation
We're starting to add more and more revision-related capabilities to our entity storage interface(s), but they do not make too much sense for config entities for example, since they are not revisionable.
- #1730874: Add support for loading multiple revisions at once wants to add a loadMultipleRevisions()
method
- #2926483: Add API methods for determining whether an entity object is the latest (translation-affecting) revision wants to add getLatestRevisionId()
method
Proposed resolution
Move loadRevision()
and deleteRevision()
out of the main EntityStorageInterface
and into a new interface: RevisionableEntityStorageInterface
.
Remaining tasks
Discuss the naming, etc.
User interface changes
Nope.
API changes
Nope.
Data model changes
Nope.
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff-40.txt | 1.41 KB | amateescu |
#40 | 2926540-40.patch | 5.4 KB | amateescu |
#36 | interdiff-36.txt | 672 bytes | amateescu |
#36 | 2926540-36.patch | 5.31 KB | amateescu |
#29 | interdiff-29.txt | 2.88 KB | amateescu |
Comments
Comment #2
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSomething like this is what I have in mind, mostly stolen from #2926483: Add API methods for determining whether an entity object is the latest (translation-affecting) revision :)
Comment #3
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAnd, of course, the major selling point of this patch is that we don't need useless method implementations on storage classes that do not support revisions.
Comment #4
plachI think this change is fine in practice, but it could theoretically be problematic: if you have code relying on
EntityStorageInterface::loadRevision()
and an implementation of the "updated"EntityStorageInterface
not defining it is passed to it, your code breaks.Shouldn't this extend
EntityStorageInterface
?Comment #5
hchonovWe would need a new base class for the new interface as well, otherwise we'll not be allowed to add new methods to it according to our BC policy. Or should we flag the new interface @internal and somehow point to
ContentEntityStorageBase
as the base class of it?Comment #6
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@plach, re #4:
1. That's a very legitimate concern, so let's just mark them as deprecated in
EntityStorageInterface
and point to the new interface instead.2. It should not extend
EntityStorageInterface
IMO, to allow for cleaner composition. A previous example of this is\Drupal\Core\Entity\RevisionableInterface
which doesn't extendEntityInterface
@hchonov, re #5:
The base class *is*
ContentEntityStorageBase
, and I don't think we want the new interface marked as @internal, because the whole point of this issue is "how do we want the storage classes to look like in D9" :)Comment #8
plachWhy isn't it called
RevisionableStorageInterface
then?Comment #10
hchonov@amateescu, oh well if this issue is not targeted against D 8.x then everything is fine :). Probably we should mention this in the issue title or summary? :)
Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@hchonov, it *is* targeted for 8.5.x. That's how we work towards 9.x, introduce APIs as we want them to be in the future, deprecate the old APIs, and remove the old ones in 9.0.0.
@plach, for no good reason :)
Comment #13
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedA less broken patch would help.
Comment #14
plachLooks good, thanks
Comment #15
plachThis is blocking a string of critical and major issues.
Comment #16
hchonovThe only problem is that once we introduce the new interface we cannot add methods to it anymore. Could we probably declare it internal until the interface is complete?
Comment #17
plachI'm fine with marking it as @internal until we are done with #2926483: Add API methods for determining whether an entity object is the latest (translation-affecting) revision and #1730874: Add support for loading multiple revisions at once, but this should not be strictly needed:
So even if we don't have a 1:1 relationship between the new interface and the base class we should be covered by the policy.
Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSure, I don't mind keeping it @internal for a short while :)
Comment #19
plachComment #20
hchonovThank you :).
I think it is worth mentioning that there is one more issue that will add a new method related to revisionable entities -
loadRevisionUnchanged
in #2620980: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load().Comment #21
BerdirHm, isn't the problem with those @deprecated that now all calls to \Drupal::entityTypeManager()->getStorage()->loadRevision() will show as deprecated, even though that's not actually the case.
I guess that also shows the downside of this, that \Drupal::entityTypeManager()->getStorage()->loadRevision() and other methods will in the future no longer autocomplete...
Comment #22
BerdirAlso, I don't see how adding @internal to an interface that is implemented by a non-internal interface (ContentEntityStorageInterface) makes any sense :)
Comment #23
hchonovContentEntityStorageInterface
could inherit anything and we are allowed to add methods to it, because it has a 1:1 relationship to a base class.It makes sense to declare the new interface internal so that custom or contrib don't implement it directly and we still could add methods to it.
Comment #24
xjmIs this actually blocking the criticals and majors? It seems like it could just as easily be a followup/cleanup, no?
I don't think we can deprecate the old methods while also marking their replacements internal. Either the deprecations should be in a followup or we shouldn't mark the interface internal.
I also strongly dislike marking interfaces internal generally because it gives the developer conflicting signals. Sometimes it makes sense but we should avoid it if it's not really needed.
Separately, about raising deprecations. In the issue that does the deprecation (whether this or another) can we raise
trigger_error()
in implementations that use the method but don't implement the new interface? Does that make sense? Edit: I may not have totally thought this through but I try to ask each time.Comment #25
plach@hchonov:
As mentioned in #17, we are allowed to add methods to interfaces even if they don't have a 1:1 relationship with a base class. If adding the @internal tag is problematic, I'd say let's just get rid of that tag and move on.
@xjm:
It's introducing the
RevisionableStorageInterface
that's required by the major/critical tasks mentioned above. Moving the methods around could definitely be a non-critical clean-up issue.@amateescu:
Based on the feedback above, especially #21 (which I verified), I'd say let's remove the deprecations and open a postponed follow-up to deprecate/remove the methods from
EntityStorageInterface
before releasing the last 8.x minor.Comment #26
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOk, how about this: instead marking them with the @deprecated tag we just put a @todo with the deprecation message and a @see tag for #2926958: Remove revision-related methods from EntityStorageInterface.
@xjm
Since all the current storage classes implement
ContentEntityStorageInterface
which extends the newRevisionableStorageInterface
, thattrigger_error()
will never be called, so it doesn't really help :)Comment #27
plach@amateescu:
Could we add the
trigger_error()
calls to theConfigEntityStorage
class?Comment #28
plachAfter quickly discussing this with @amateescu in IRC, we agreed that since there's no actual deprecation, we cannot trigger errors.
We also agreed to release this joint statement:
Comment #29
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#1730874: Add support for loading multiple revisions at once just got in! Which means that we need to move that method to the new interface, and we don't have to worry about BC because it's just in the dev branch.
Comment #30
xjmLOL how can I deny #28?
So it looks like these empty implementations were just added in the other issue, and the idea is that they were only added because they were required for the
base classnon-revisionable implementations when they were on the other interface, and we hadn't yet split off this interface. Is that accurate?Comment #31
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@xjm, yup, very accurate :)
Comment #32
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWe also need a change record for this new interface but I want to postpone writing/publishing that CR until we actually define all of its methods..
Comment #33
xjmHmm, okay, not sure about doing that. It's docs gate so we shouldn't really postpone it. We can say in the CR that the interface is still being extended and that we'll add to it later.
I just looked at #2926958: Remove revision-related methods from EntityStorageInterface and it's filed as being for 9.x. But we should actually add a
@deprecated
in 8.x (right?). Because anyone not extending a core base class still needs that information, and that's also what we'll be grepping for when we go to open 9.x. Sorry if there was some confusion from #24; trying to figure out how we couldtrigger_error()
for interface methods doesn't need to be in scope here. (We have the same issue for constants.) But the notice itself can follow the standard form.Sorry, sad-eyed kitty, I do think it needs to be NW. :) Hopefully not too painful though since it looks like we're in CI limbo ATM anyway.
Comment #34
plachQuickly discussed #33 with Jess, we are ok with leaving the @todos and opening a follow-up to update our policies to address cases like this one.
@amateescu:
We need to move this to
KeyValueContentEntityStorage
, which should fix those failures.I'm going to take care of the CR and the follow-up.
EDIT: I think it's fine to leave the CR in draft status until we're done with these refactorings, more or less what Hristo was suggesting above with the @internal tag.
Comment #35
plachFollow-up to discuss policy changes at #2927251: [policy, no patch] Decide how to deal with methods needing to be moved down an interface hierarchy.
Draft CR at https://www.drupal.org/node/2927226.
Comment #36
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks @plach!
Here's the small update needed to fix #29.
Comment #37
xjmCan we add a second @see to the CR? So that it's still sorta like other deprecations even though we're using an @todo.
Nit: ID unless we're talking about Freudian psychology. ;)
Comment #38
xjmMaybe we were making this too complicated...NM, we're deprecating the method on the other parent, but it still raises warnings in IDEs for the child.
Comment #39
xjmPatch diff is also a little confusing; looks like part might have been staged and the other part unstaged.NM, missed that there are two KV imlementations.
Comment #40
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFixed #37.
Comment #41
plachYay
Comment #43
xjmOK I think this is sufficient given the followups and that it's blocking other things. Committed and pushed to 8.5.x, and published the CR. Thanks!