Problem/Motivation

Currently it is impossible to check from outside the entity if it has been validated.

Proposed resolution

Introduce ContentEntityInterface::isValidated().

Remaining tasks

User interface changes

API changes

Data model changes

Comments

hchonov created an issue. See original summary.

hchonov’s picture

Status: Active » Needs review
FileSize
2.16 KB
hchonov’s picture

I've forgotten to install the entity schema and therefore the previous test is failing.

The last submitted patch, 2: 2890333-2.patch, failed testing. View results

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Can't see any reason not to do this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2890333-3.patch, failed testing. View results

hchonov’s picture

Status: Needs work » Reviewed & tested by the community

It has been a random failure. Setting back to RTBC.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
@@ -71,4 +71,12 @@ public function getLoadedRevisionId();
+  public function isValidated();

We can't add methods to an interface - its a BC break.

hchonov’s picture

No, it is not an API break. We are allowed to add new methods to interfaces, where a base class : interface relationship is provided and this is the case between ContentEntityBase and ContentEntityInterface. It is not an API break because according to our policy developers should extend from the base class instead implementing the interface. From https://www.drupal.org/core/d8-bc-policy:

Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features.

Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.

And we've done this a lot of times already just look at the git annotations in e.g. ContentEntityInterface :).

larowlan’s picture

Status: Needs work » Needs review

Hmm, I wasn't aware of that - it seems counter intuitive.

Will check with another committer.

Thanks for pointing it out.

catch’s picture

The documentation for that is here: https://www.drupal.org/core/d8-bc-policy#interfaces

We decided on allowing that (where there's a base class we expect every implementation to inherit from) to avoid having to add a new interface (and deprecation) every time we add a new method. In theory this would seem more disruptive than adding new interfaces, but in practice it's worked out well so far.

larowlan’s picture

Issue tags: +Needs change record

Ok, can we have a change record describing the new functionality.

Then I think it's ready

Thanks

hchonov’s picture

Issue tags: -Needs change record

I've just created the change record - https://www.drupal.org/node/2895323.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Think we can go back to RTBC then?

timmillwood’s picture

Should the change record provide a possible use case or a real world example? I often find it much easier to understand something with an example.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record updates

The reason we need the CR is because of the BC break, so the CR should cover that. The method is being added to ContentEntityInterface. ContentEntityBase includes a default implementation, so most content entity definitions won't be affected; however, anyone implementing ContentEntityInterface directly will now need to implement this method as well.

This actually makes me wonder if this is the right interface for this method. The parent interface FieldableEntityInterface is the one that provides the concept of validation. Should this method go there instead?

hchonov’s picture

@xjm, like pointed out in #9 and in #11 we add new methods to a base class where a 1:1 relationship interface : base class is provided. Adding the method to ContentEntityInterface is not considered an API or BC break, but adding it to FieldableEntityInterface is considered API break. This is why we add new methods to ContentEntityInterface instead to FieldableEntityInterface and in D9 we have to move the methods to the proper interfaces.

From #2862574-11: Add ability to track an entity object's dirty fields :

We just had a discussion about the new interface on IRC and personally at the DrupalDevDays Seville with tstockler and berdir and they both agree that we add new methods to content entities by adding them to ContentEntityInterface and in D9 they should be moved to the according interface - in this case to FieldableEntityInterface. We've done this for an example with the getLoadedRevisionId method.

This means we do not need the new interface and instead we have to add the new method to ContentEntityInterface.

hchonov’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs review » Needs work

That makes sense. Thanks @hchonov. If the plan is to move them to the proper interface in D9, then let's please put @todos on them and create an explicit followup. (Also @see to the validation methods on the parent interface, because I had to go look for them.)

I thought a minute about how this would work in the continuous upgrade path. There isn't a BC break for implementors of CEI because they inherit the method when it's moved to the parent. But direct implementations of FEI are disrupted in this D9 change. I don't really think there's any way to mark a deprecation for this. Any thoughts on how we inform developers in advance in the continuous upgrade path model?

The CR updates will still be needed then also. We can include this information about moving the method in the future in the CR as well.

Thanks!

catch’s picture

Different ways this could be done with the continuous upgrade path stuff:

1. Add the deprecation on ContentEntityInterface for 9.0.0 to be removed in 10.0.0

2. A new ContentEntityInterface in a different namespace with less methods, then ContentEntityInterface inherits from it in 8.x and not in 9.x

3. A new FieldableEntityInterface with the additional method. Have ContentEntityInterface inherit from the new interface, which in turn inherits from the old interface.

The trouble with #1 is that it's time-bound between opening 9.x and and releasing 9.0.0, and that it leaves the method until 10.0.0

The trouble with #2 is that for each method we want to remove from ContentEntityInterface, we could end up with a new interface in 8.x.

Trouble with #3 is it requires an instanceof check where FieldableInterface is type hinted and you want to use the new method, and it has the interface-proliferation drawback of #2.