Create EntityPublishedInterface and use for Node and Comment

In #2725463: WI: Add StatusItem field type to store archived state we're looking to overhaul the status field, but I don't think the case for this interface would change.

If anything I might suggest changing the interface to EntityStatusInterface, so we can add in there new things relating to the status field, but not relating to published.

User also has a similar concept via 'created', 'active', 'blocked'. EntityStatusInterface would be more suited to that as well.

@catch - right, but isPublished() and setPublished() wouldn't make much sense on users, would it?

It wouldn't but something like getStatus() and setStatus() might?

No more 8.1.x releases are going to be made, and as this is not a bug it won't go in 8.2.x either, so switching to 8.3.x.

I still think the interface should be named EntityStatusInterface.

I think also that this should be EntityStatusInterface with methods getStatus() and setStatus().

I think the proper way to do this while retaining backwards compatibility, is to provide the new methods on the comment and node classes, and deprecate isPublished() and setPublished(), with a message that the new methods should be used instead.

I accidentally opened a new issue for this #2811667: Introduce EntityPublishedInterface for content entities where I have contradicted my statements in this issues.

If the status field in Node and Comment are starting a boolean published / unpublished field I think we'd be better of with EntityPublishedInterface for them, then maybe for config entities we add EntityStatusInterface which is pretty much what's being discusses in #2009958: Introduce EntityStatusInterface to standardize how entities can be disabled.

Pulling over the latest patch from #2811667: Introduce EntityPublishedInterface for content entities.

This patch defines EntityPublishedInterface to be used for all entity types wanting a published/unpublished status. It also creates generic constants rather than having separate ones currently in use by node and comment modules.

Now that #2810381: Add generic status field to ContentEntityBase has been committed we should get this in too.

Should EntityPublishedTrait implement EntityPublishedInterface, or just the entity type class which is using the trait?

1. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedInterface.php
   @@ -0,0 +1,42 @@
   + * Provides an interface for access to an entities published state.
   

   entity's.

2. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedInterface.php
   @@ -0,0 +1,42 @@
   +  const STATUS_PUBLISHED = 1;
   

   Does the STATUS_ prefix gain us anything?

3. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedInterface.php
   @@ -0,0 +1,42 @@
   +  public function setPublished($published);
   

   I wonder a bit whether this should be function publish() and function unpublish() with no params. That's not how Node works now though.

   Also the param is bool, but we define the constants as int - is a bit confusing. If we removed the param from the function, it'd at least partially reduce that confusion, although we still have the bool return value from isPublished() vs. the constants.

1) Will update
2) In CommentInterface we already have const PUBLISHED, I was getting override issues if we had CommentInterface and EntityPublishedInterface both having the same constants.
3) I didn't really want to change the existing functionality, just provide a common interface for it. I'm happy to add publish() and unpublish() in addition to the traditional setPublished($published). Then with the bool vs. int, I think this is just a Drupalism we embrace until D9.

#16.2) Can't we just remove the constants from CommentInterface since it will inherit them from the new interface?

@amateescu - Hrm... I suppose you're right.

Fixes #15, updated based on EntityPublishedTrait, and trying suggestion from #18.

Should EntityPublishedTrait implement EntityPublishedInterface, or just the entity type class which is using the trait?

Traits can't implement interfaces, so it'll just be the entity type classes doing it as well as using the trait.

@catch - Yes, I worked this out when I tried it. Still getting used to the OOP world.

1. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedInterface.php
   @@ -0,0 +1,57 @@
   +  /**
   +   * Returns the entity published status indicator.
   +   *
   

   I would rename this to:

   Returns whether or not the entity is published

   The way it is currently worded I would expect to get back some kind of PublicationStatusIndicator object.

2. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedInterface.php
   @@ -0,0 +1,57 @@
   +   * Unpublished entities are only visible to their authors and to
   +   * administrators.
   

   Is this true under all circumstances? This is copied from the current implementation but this would not necessarily be true for all kinds of entities.

   For example an entity might not have an author associated with it, but it might have a publication state.

3. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -41,20 +32,32 @@ public function isPublished() {
   +    /** @var \Drupal\Core\Entity\ContentEntityTypeInterface $entity_type */
   

   Why is this type hint for $entity_type here? This variable is not defined in this scope.

4. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -41,20 +32,32 @@ public function isPublished() {
   +    $key = $this->getEntityType()->getKey('status') ?: 'status';
   

   I think we should probably throw an exception if the status key is not defined, rather than just blindly overwriting the 'status' field. It's quite a remote possibility but this might cause data loss.

   This is also a bit ambiguous. We are now making a clear difference between 'published' and 'status', but we are still relying on the entity key called 'status'. This will come back to bite us in #2009958: Introduce EntityStatusInterface to standardize how entities can be disabled.

   This needs a bit more thought I guess.

5. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -41,20 +32,32 @@ public function isPublished() {
   +    /** @var \Drupal\Core\Entity\ContentEntityTypeInterface $entity_type */
   

   Same here, $entity_type is not defined.

Fixed items from #23, apart from 4. In #2810381: Add generic status field to ContentEntityBase we introduced EntityPublishedTrait and after working it through with Plach we decided that publishing status fields may or may not have an entity key. For example Node does, but comment doesn't. so we support a 'status' entity key if there is one, otherwise we just just a field called 'status'.

I quite like the suggestion of removing setPublished() and the constants from the interface. publish() and unpublish() are sufficient.

I wouldn't split it in separate interfaces though, that kind of granularity seems overkill to me.

#2009958: Introduce EntityStatusInterface to standardize how entities can be disabled is a very different issue and relates to the status field on config entities. Here we're looking at the publishing status on content entities.

I don't think we need two interfaces for publish and unpublish.

setPublished() is heavily in use, and is often good if we want to set the publishing status based on the result of something. $entity->setPublished($published); is nicer than $published ? $entity->publish() : $entity->unpublish(); in my opinion.

I'm very happy with the code in #24.

Right, common sense will stop people putting non-boolean things into setPublished(). Also note, we are not planning to remove the method, just not add it to this interface.

Also, there are no plans for the publishing status to be anything more than boolean. We did discuss it and decided it's not viable. So EntityPublishedInterface only ever needs to support publishing and unpublishing.

Rather than...

$published = $dependency->getSomeBooleanResult($entity);
if ($published) {
  $entity->publish();
}
else {
  $entity->unpublish();
}

as you suggested,

$published = $dependency->getSomeBooleanResult($entity);
$entity->setPublished($published);

would be a lot clearer.

I'm not saying it's either / or, lets have the traditional setPublished() and the new publish()/unpublish().

I looked around the code base a bit and found out that from 32 usages of setPublished() only 4 actually set it based on an external value, so I think it's ok to keep setPublished() out of the new interface.

I agree with #30 that having three setters would be suboptimal DX, and we also have to keep in mind that the new interface has to make sense on its own and that we shouldn't architect it based on past decisions that maybe made sense at some point :)

Based on the comment above, just moving things along a bit :)

1. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -30,31 +24,45 @@ public static function publishedBaseFieldDefinitions(EntityTypeInterface $entity
   -   * Sets the entity as published or not published.
   -   *
   -   * @param bool $published
   -   *   A boolean value denoting the published status.
   -   *
   -   * @return \Drupal\Core\Entity\ContentEntityInterface $this
   -   *   The Content Entity object.
   +   * {@inheritdoc}
   

   setPublished() is no longer on the interface, so we should replace the {@inheritdoc} with the full docblock again.

2. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -30,31 +24,45 @@ public static function publishedBaseFieldDefinitions(EntityTypeInterface $entity
   +  public function unpublish() {
   +    $key = $this->getEntityType()->getKey('status') ?: 'status';
   

   I mentioned this before in my review of comment #23, but I think I did not properly communicate what troubles me about this.

   Using the status entity key here, and defaulting to an entity property $entity->status with hard couple the concept of an entity being "published" with its "status".

   This conflicts with #2009958: Introduce EntityStatusInterface to standardize how entities can be disabled where we want to introduce a EntityStatusInterface.

   My concern is that if we go ahead here and hard couple publication state to this entity key, then we will not be able to do #2009958: Introduce EntityStatusInterface to standardize how entities can be disabled without introducing a B/C break, effectively condemning that issue to be postponed to D9.

   I suggest we instead introduce the entity key published for this, and in the Node and Comment entity definitions we declare:

     entity_keys = {
       "status" = "status",
       "published" = "status",
     },
   

   This will ensure that we still will be able to get an EntityStatusInterface in a reasonable timeframe.

+++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
@@ -11,33 +11,28 @@
-  public static function publishedBaseFieldDefinitions(EntityTypeInterface $entity_type) {
-    $key = $entity_type->hasKey('status') ? $entity_type->getKey('status') : 'status';
-    return [$key => BaseFieldDefinition::create('boolean')
+  public static function publishedBaseFieldDefinitions(EntityTypeInterface $entityType) {
+    $definitions = [];
+
+    $key = self::getPublishedEntityKey($entityType);
+    $definitions[$key] = BaseFieldDefinition::create('boolean')
       ->setLabel(new TranslatableMarkup('Publishing status'))
       ->setDescription(new TranslatableMarkup('A boolean indicating the published state.'))
       ->setRevisionable(TRUE)
       ->setTranslatable(TRUE)
-      ->setDefaultValue(TRUE)];
+      ->setDefaultValue(TRUE);
...
+    return $definitions;

Apart from making use of getPublishedEntityKey() I'm not sure this is the issue to change this (if it needs changing at all).

updating patch based on #37.

I like the latest version of the interface, nice and clear.

I wanted to check to what extent 'unpublish' is used. It's something we already use a lot, but further reinforced here. However I did a google and found this question from kiamlaluno on a site, and the answers were in favour of unpublish vs. withdraw/retract. http://english.stackexchange.com/questions/2208/does-the-verb-unpublish-...

So +1 to the patch as it currently is.

Nice API change. Some comments...

1. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedInterface.php
   @@ -0,0 +1,34 @@
   +   * @return \Drupal\Core\Entity\EntityInterface
   +   *   The called entity.
   ...
   +   * @return \Drupal\Core\Entity\EntityInterface
   +   *   The called entity.
   

   We usually document this as @return $this. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a....

2. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -11,16 +11,10 @@
      /**
   -   * Returns an array of base field definitions for publishing status.
   -   *
   -   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   -   *   The entity type to add the publishing status field to.
   -   *
   -   * @return \Drupal\Core\Field\BaseFieldDefinition[]
   -   *   Array of base field definitions.
   +   * {@inheritdoc}
       */
   

   I don't see what docs is this inheriting, it has no parent. Revert?

3. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -11,16 +11,10 @@
   +    $key = self::getPublishedEntityKey($entityType);
   

   s/self::/static::/ ?

4. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -30,14 +24,11 @@ public static function publishedBaseFieldDefinitions(EntityTypeInterface $entity
   +    $key = $this->getPublishedEntityKey($this->getEntityType());
   
   @@ -50,12 +41,49 @@ public function isPublished() {
   +    $key = $this->getPublishedEntityKey($this->getEntityType());
   ...
   +    $key = $this->getPublishedEntityKey($this->getEntityType());
   

   This is correct but why not calling it as static s/$this->/static::/ ?

5. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -50,12 +41,49 @@ public function isPublished() {
      public function setPublished($published) {
   

   We should deprecate also this, I think.

6. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -50,12 +41,49 @@ public function isPublished() {
   +   * @return string
   +   *   The configured entity type definition key for 'status', or defaults to
   +   *   'status'.
   +   * @see \Drupal\Core\Entity\Annotation\ContentEntityType
   

   Missing new line between @return and @see.

7. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -50,12 +41,49 @@ public function isPublished() {
   +    return $entityType->getKey('status') ?: 'status';
   

   Hm, I don't like this. Why 'status' for free? If an entity type is implementing this interface and misses a 'status' key then it's a malformed entity type, right? So, we need to throw something here.

8. +++ b/core/modules/node/node.module
   @@ -14,7 +14,9 @@
   +use Drupal\Core\Entity\ContentEntityBase;
   ...
   +use Drupal\Core\Entity\EntityPublishedInterface;
   

   Unused statements.

Resolved everything apart from 7.

Not all entity types use, or should use a 'status' entity key. Take the current core examples of Node and Comment entities. Node has a 'status' entity key, and Comment doesn't. When working on #2810381: Add generic status field to ContentEntityBase Plach questioned the entity key, because I originally planned to add it to all entity types that want to use the 'status' field, but it seems there is no need. Therefore with this trait, if there is an entity key we use it, if not it's assumed the fields is 'status'. Hopefully the base field from the trait is used too, and not just the other methods in the trait.

Plach questioned the entity key, because I originally planned to add it to all entity types that want to use the 'status' field, but it seems there is no need. Therefore with this trait, if there is an entity key we use it, if not it's assumed the fields is 'status'.

Why? I see that entity types should be very carefully defined with all the keys in place. I don't understand this exception. If we miss the "id" key from the definition, does it have such a hard-coded fallback? (if it has, it's wrong too)

I see now that @pfrenssen raised the same concern. Let's wait for more input on this.

Discussing with @timmillwood on IRC. It's true that we should keep the hard-coded 'status' for assuring BC. If a custom entity type didn't add the key, relying on this hard-coded fallback, probably it will crash. For this reason we should keep that but add a @todo to be removed in D9?

I think comparing this to the entity_key for 'id' is a little different, as 'id' is set in ContentEntityBase, where as this is in a trait. This is more similar to \Drupal\Core\Entity\RevisionLogEntityTrait where all the field keys are hard-coded. Although there is an issue open to define these keys in the entity annotation, not using entity_keys, but another property.

Discussed the 'status' entity key problem briefly in IRC today and both @catch and @FabianX were in favor of introducing a 'published' entity key.

Here's how it would look like.

@GroovyCarrot, the generic public set() and get() methods are needed for configurable fields, which can not have dedicated methods on the entity interfaces.

To be honest, I have no idea why using the 'status' field as an entity key in Comment would impact the output of HAL+JSON vs. JSON in the rest module, but this fixes the test.

1. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -17,11 +18,21 @@
   -    $key = $entity_type->hasKey('status') ? $entity_type->getKey('status') : 'status';
   -    return [$key => BaseFieldDefinition::create('boolean')
   +    if (!is_subclass_of($entity_type->getClass(), EntityPublishedInterface::class)) {
   +      throw new InvalidEntityTypeDefinitionException('The entity type ' . $entity_type->id() . ' does not implement \Drupal\Core\Entity\EntityPublishedInterface.');
   

   This is a BC breaking change for someone whoal already used this trait. But it's against trait that is new in 8.3.x and was never part of a release.

   I *suppose* that is OK, no idea?

   I was following the discussion around this a but to be honest, I don't fully understand it. Yes, status can mean something other than published, especially for config entities. But having a published or not "status" and another status seems more than unlikely? And if you do have that, you can always not use this trait?

2. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -46,15 +54,40 @@ public function isPublished() {
   +   * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0. Use
   +   *   \Drupal\Core\Entity\EntityPublishedInterface::publish() and
   +   *   \Drupal\Core\Entity\EntityPublishedInterface::unpublish() instead.
       */
      public function setPublished($published) {
   

   Here we deprecate this but I suppose what we actually deprecate is the existing method on Node and Comment?

3. +++ b/core/lib/Drupal/Core/Entity/Exception/InvalidEntityTypeDefinitionException.php
   @@ -0,0 +1,8 @@
   +/**
   + * Defines an exception thrown when an entity type definition is invalid.
   + */
   +class InvalidEntityTypeDefinitionException extends \Exception { }
   

   naming nitpick: invalid is something that I'd expect we throw on discovery when the definition is.. invalid. the way it is used feels more like incomplete/unsupported entity type definition. but that's pretty minor.

4. +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -81,8 +83,12 @@ public function preSave(EntityStorageInterface $storage) {
        if (is_null($this->get('status')->value)) {
   -      $published = \Drupal::currentUser()->hasPermission('skip comment approval') ? CommentInterface::PUBLISHED : CommentInterface::NOT_PUBLISHED;
   -      $this->setPublished($published);
   +      if (\Drupal::currentUser()->hasPermission('skip comment approval')) {
   +        $this->publish();
   +      }
   

   wondering if we should deprecate those constants as well as it is now officially a boolean value? happy to discuss that in a follow-up issue.

5. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -311,7 +311,6 @@ public function testUpdateComment() {
        $comment->setSubject('Initial subject')->save();
        $read_only_fields = [
   -      'status',
          'pid', // Extra compared to HAL+JSON.
   

   this is strange, I think we can't just remove this without at least knowing what is happening?

This is a BC breaking change for someone whoal already used this trait. But it's against trait that is new in 8.3.x and was never part of a release.

I *suppose* that is OK, no idea?

Yes that's OK, we could also have rolled back the trait entirely before 8.3.0.

I was following the discussion around this a but to be honest, I don't fully understand it. Yes, status can mean something other than published, especially for config entities. But having a published or not "status" and another status seems more than unlikely? And if you do have that, you can always not use this trait?

File entities are a particularly strange one, that in theory at least could have a concept of being published vs. not. Users have a status key which isn't the same as published, but don't see us adding published.

I could see us renaming the schema for node etc. to use 'published' instead of 'status' in 9.x so it all lines up (i.e. we'd somehow deprecate direct calls to set('status'), not sure how to do a forward compatible change for entity queries but it could be mapped in an alter or something), in that case having published as the entity key helps, I see the ambivalence towards it now.

#2 yes with with the trait itself being new, we should remove that method rather than deprecate it, and just deprecate the node/comment methods instead.

Thanks for reviewing :)

1. As @catch said, I also assumed it's ok to do it since the trait is new.
2. Removed the method from the trait and brought back the ones in Node and Comment, while keeping them deprecated in the interfaces.
3. No preference here, modified according to your suggestion.
4. I don't think the constants are actually deprecated. The new interface doesn't care if you're using constants or booleans in the storage, but the existing implementations for Node and Comment do assume that integers are used. (e.g. for entity query purposes)
5. Left a Druplicon tell to @Wim Leers on IRC to see if he can shed some light on this ;)

About the status change:

First, the previous issue didn't add status to read only fields, it moved it. Because we now define it earlier up, the access check for it happens first. And the read only fields must be in that order, as it checks them one by one to figure out if you really don't have access. So it encountered the status first.

And here's what's happen with the status, this piece of cause is the "problem":

// Entity key fields need special treatment: together they uniquely
// identify the entity. Therefore it does not make sense to modify any of
// them. However, rather than throwing an error, we just ignore them as
// long as their specified values match their current values.
if (in_array($field_name, $entity_keys, TRUE)) {
// Unchanged values for entity keys don't need access checking.
if ($original_entity->get($field_name)->getValue() === $entity->get($field_name)->getValue()) {
debug($field_name, 'skip');
continue;
}

status is an entity key (btw, the above assumption is bogus, entity keys are *not* "what uniquely identify the entity") now, so it is checked as "did the value change or not", and if not, then we skip it and don't check access. Weird, but that's what's happening ;)

So because don't actually try to *change* the status, it is no longer access denied but silently ignored.

#46 + #48:

To be honest, I have no idea why using the 'status' field as an entity key in Comment would impact the output of HAL+JSON vs. JSON in the rest module, but this fixes the test.

There is a very good reason for this. In PATCH request, one should send the fields one wants to change. But of course one cannot change entity keys: you cannot change the entity ID for example. However, there's an edge case: some entity keys must be sent for an entity to be denormalizable at all: the bundle field! So, entity keys are whitelisted since #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it: they are allowed to be sent, but they must NOT be changed: they must match the currently stored value.

So, if you're making a field that was not an entity key into an entity key, that means that if the field access check for that field did not allow changes before, then it won't complain anymore this time around. That's what is happening.

But why is this being made into an entity key?

#52: hah, only after having written all of the above do I see that you've already basically answered this. But, at least I was able to explain why it behaves this way.

btw, the above assumption is bogus, entity keys are *not* "what uniquely identify the entity"

Interesting! So what are entity keys for then? I'm not able to find relevant docs.

Oh, found it:

  /**
   * Holds untranslatable entity keys such as the ID, bundle, and revision ID.
   *
   * @var array
   */
  protected $entityKeys = array();

Wow, that is super confusing. Very bad name? Even more so because it is then followed by:

  /**
   * Holds translatable entity keys such as the label.
   *
   * @var array
   */
  protected $translatableEntityKeys = array();

So then what is "key" about these keys?

But of course one cannot change entity keys: you cannot change the entity ID for example.

You can change entity keys as much as you want actually. What you should be looking at is the read-only property of a field definition (i.e. $field_definition->isReadOnly()) ;)

Edit:

But why is this being made into an entity key?

In order to bring consistency into how EntityPublishedTrait defines the name of the 'status' field.

Discussed this a bit with berdir in IRC:

berdir> WimLeers: $entityKeys and $translatableEntityKeys are the values of those entity keys for fast access.
11:31:51 <WimLeers> berdir: so is it just for fast access then?
11:32:03 <berdir> WimLeers: there isn't really any documentation what they are. They basically have only one purpose: To allow generic access to a common concept without harcoding a field/property name
11:32:18 <WimLeers> berdir: ughhhh
11:32:32 <WimLeers> berdir: but this basically means \Drupal\rest\Plugin\rest\resource\EntityResource::patch is incorrect
11:33:25 <berdir> So if anything then rest should not do that logic for all entity keys. either it should do it for *all* fields (sending a value without changing it does not require write permission. but could this could allow you to figure out the value of a field you are not allowed to see or change. just try until you don't get an error) or only specific, actually read-only keys like id and bundle
11:34:02 <berdir> WimLeers: well. if it makes you fell better, it is not alone. entity storage makes a similar wrong assumption and makes fields that have an entity key pointing to them required/NOT NULL in the schema
11:34:30 <WimLeers> berdir: right, I remember researching this in depth and seeing it somewhere
11:34:40 <WimLeers> berdir: that's also why nobody noticed it while working on that patch
11:34:51 <WimLeers> berdir: it's interesting how REST unveils more flaws in Entity API
11:35:00 <WimLeers> berdir: because form-based assumptions are no longer present
11:35:18 <WimLeers> berdir++
11:35:22 <berdir> WimLeers: I think a better thing to do would be to respect the read-only property of the field definitions, see \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions()
11:35:41 <berdir> WimLeers: which nothing in core actually uses yet
11:36:27 <berdir> WimLeers: what is interesting there though is that revision_id is also marked as read only. but the API actually allows you to save a new revision with a specific revision id, but that's basically just for migrate, nobody else should rely on that :)
11:36:37 <berdir> and it seems like a sane assumption to not allow that over rest
11:36:50 <WimLeers> berdir: but that's the thing, we dont' really want to whitelist every read-only thing. We just want the critical things to be sent. Nothing more. Although this would actually be even better, from the "be liberal in what you accept, be conservative in what you send" principle (https://en.wikipedia.org/wiki/Robustness_principle)
11:37:00 <WimLeers> berdir: oh heh
11:37:04 <WimLeers> berdir: yes please no :D
11:37:13 <WimLeers> berdir: but then how can we automatically detect that?
11:38:05 <berdir> WimLeers: I think we should at least try what happens if we switch that logic to checking the read only property instead. this makes much more sense to me than the current entity key based code. can you open an issue? :)
11:38:08 <WimLeers> +1

… and because of this, this means that what \Drupal\rest\Plugin\rest\resource\EntityResource::patch() does is wrong, and so the changes in #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it were wrong.

This means it won't be possible anymore to modify the status field of a Comment entity via REST. This needs to be fixed before this issue can get in. Created a separate issue: #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.

d.o, you're stupid.

So d.o ate my title change and decided to change the status of this node. I definitely did not touch that. d.o has an appropriately sarcastic on-topic sense of humor!

Republished.

11:49:19 <amateescu> but why is it a blocker?
11:49:36 <amateescu> IMO it's a followup at best
11:49:38 <WimLeers> amateescu: like I said on the issue: "This means it won't be possible anymore to modify the status field of a Comment entity via REST. "
11:49:46 <WimLeers> amateescu: no, because it would cause a regression in REST
11:50:05 <berdir> yes it is
11:50:08 <berdir> if you have permission
11:50:22 <berdir> WimLeers: the code skips the permission access *only* if the value didn't change
11:50:23 <WimLeers> berdir: ?
11:50:31 <berdir> so if you actually change the value and have permission, you can still change it

Berdir is right.

But then I'd like to see this add test coverage to prove that it's still possible to change a comment's status.

But then I'd like to see this add test coverage to prove that it's still possible to change a comment's status.

Don't we have a dedicated issue for that? #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method

That issue is about a massive testing overhaul. This would indeed eventually end up in there, as a special case to test for Comment entities. But that patch is not even RTBC yet. For now, this patch would need to expand the test coverage in Drupal\rest\Tests\UpdateTest::testUpdateComment().

This issue is currently not blocked so we are looking for reviews of #51.

We have been through a number of iterations, therefore it should be in a pretty good place.

+++ b/core/modules/rest/src/Tests/UpdateTest.php
@@ -311,7 +311,6 @@ public function testUpdateComment() {
-      'status',

Well, this change is wrong.

If this change were correct, then we'd also see this change in #2826101: Add a ReadOnly constraint validation and use it automatically for read-only entity fields.

So AFAICT it is blocked. :(

It is the correct change as that is what rest.module currently does. We could add a @todo to that issue but I agree that we don't need to block on that.

No, it is wrong.

  protected function checkFieldAccess($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
    if ($operation == 'edit') {
      // Only users with the "administer comments" permission can edit
      // administrative fields.
      $administrative_fields = array(
        'uid',
        'status',
        'created',
        'date',
      );
      if (in_array($field_definition->getName(), $administrative_fields, TRUE)) {
        return AccessResult::allowedIfHasPermission($account, 'administer comments');
      }

The test change proves that this is a BC-breaking regression: with this patch committed, a different (incorrect) response is returned for PATCH /comment/CID requests whose request body includes the status field set to the same value as the current value when the user does not have the administer comments permission. This will then later change again, once #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior lands: that will restore current HEAD's behavior.

If this lands now, there is no guarantee that #2824851 will also land before 8.3. Hence we might ship with the behavior introduced in this patch in 8.3, and then change it again in 8.4. That's why this affects BC. That's why this needs to be blocked.

Committing this means Drupal 8 is API-second, not API-first.

1. +++ b/core/lib/Drupal/Core/Entity/Exception/UnsupportedEntityTypeDefinitionException.php
   @@ -0,0 +1,8 @@
   +class UnsupportedEntityTypeDefinitionException extends \Exception { }
   

   Nice. This will be helpful also in other places.

2. +++ b/core/modules/comment/comment.install
   @@ -194,5 +194,17 @@ function comment_update_8300() {
   +function comment_update_8301() {
   
   +++ b/core/modules/node/node.install
   @@ -234,3 +239,19 @@ function node_update_8300() {
   +function node_update_8301() {
   

   Both updates need testing. As they are separate modules, I'm afraid that we need two tests.

If this lands now, there is no guarantee that #2824851 will also land before 8.3. Hence we might ship with the behavior introduced in this patch in 8.3, and then change it again in 8.4. That's why this affects BC. That's why this needs to be blocked.

What stops #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior from being committed to 8.3.x? It is marked as a major bug as far as I see.

Committing this means Drupal 8 is API-second, not API-first.

Thanks for the drama, I now have 0 (zero) intentions on working on that issue anymore.

@claudiu.cristea, thanks for reviewing and good point, added two update tests :)

Nit

+++ b/core/modules/node/src/Tests/Update/NodeUpdateTest.php
@@ -0,0 +1,41 @@
+      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.bare.standard.php.gz',

I think it should be drupal-8-rc1.bare.standard.php.gz. That is newer.

I think it should be drupal-8-rc1.bare.standard.php.gz. That is newer.

That's true, but it doesn't really matter for the purpose of our test..

Edit: I'll update it if there's any more feedback to address, I don't like patch spamming :)

#66:

1. Nothing stops it from being committed. But as the thousands of bugs that Drupal has show, it's not guaranteed that a bugfix is committed to a particular release.
2. There was no drama. There was only a simple factual statement. You chose to interpret it as drama.

Ok, no other feedback for a week, so let's move forward here :)

Here's the change requested in #68.

From my point of view this is RTBC. I will let core committers to make the final decision. RTBC if green!

Back to RTBC after a round of random fails.

+++ b/core/lib/Drupal/Core/Entity/Exception/UnsupportedEntityTypeDefinitionException.php
@@ -0,0 +1,8 @@
+/**
+ * Defines an exception thrown when an entity type definition is invalid.
+ */
+class UnsupportedEntityTypeDefinitionException extends \Exception { }

Should this be \LogicException or at least something more specific?

Unrelated failures.

+++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
@@ -17,11 +18,21 @@
+    if (!is_subclass_of($entity_type->getClass(), EntityPublishedInterface::class)) {

Why is_subclass_of instead of instanceof here?

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -81,8 +83,12 @@ public function preSave(EntityStorageInterface $storage) {
+      if (\Drupal::currentUser()->hasPermission('skip comment approval')) {
+        $this->publish();
+      }
+      else {
+        $this->unpublish();
+      }
     }

Looking at this, the publish()/unpublish() methods look to me like you don't need to also call save(). I think we should do setPublished()/setUnpublished() so it's more clear that these are setters/getters. Sorry for not catching that earlier. Just a rename, it's still nice to not have to use the bool constants.

instanceof needs an object, $entity_type is the annotation object that returns the entity class name. That's why we added isSubclassOf() but that is equally confusing: #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements().

The problem with setPublished() is that it conflicts with the existing setPublished($published) from NodeInterface and CommentInterface :/

OK I was stumped for a while, but have two ideas which might be viable:

1. The bc policy allows us to change the API for interfaces for implementers not callers.

Interfaces follow a similar pattern as above with respect to @api, @internal, or neither. However, in case of neither tag, the interface is treated as an API for callers but not for implementers.

We could therefore potentially remove the argument from NodeInterface, but have Node itself do a func_get_args() and handle it. That would be no meaningful break for anyone, unless someone has directly implemented NodeInterface without subclassing Node (and it'd be a very small change even if they did).

2. To have no bc break at all, we could keep the bool argument to setPublished() in the new interface but immediately deprecate it.

So EntityPublishedInterface::setPublished($published = NULL). The trait can then handle the boolean if it's set, but also do @trigger_error('...', E_USER_DEPRECATED) etc. then we remove the argument in 9.x.

Thought more about the 8.x-9.x bc break with #85.2. Because of the trait, as long as implementers of the interface use the trait, then we can remove the param from the interface + trait in 9.0.0, and any module using the trait neither has to update a method signature nor any use statements - so it should be a completely forward-compatible fix. Option #1 would be better for direct implementors of EntityPublishedInterface but even if you did that, you could still update to use the trait when 9.x is out and have cross-compatibility.

I have to say that after coding #85.2, it actually looks pretty good and reasonable :)

Nice. Much more better.

Nits

1. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedInterface.php
   @@ -0,0 +1,37 @@
   +   * @param bool $published
   

   Then @param bool|null $published

2. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedInterface.php
   @@ -0,0 +1,37 @@
   +   *   (optional and deprecated) TRUE to set this entity to published, FALSE to
   

   (optional and deprecated) hm, I have no better idea :)

3. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedInterface.php
   @@ -0,0 +1,37 @@
   +   *   Drupal 8.3.0 and will be removed in Drupal 9.0.0.
   

   "...removed in 9.0.x" or "...removed before 9.0.0".

4. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -30,31 +41,37 @@ public static function publishedBaseFieldDefinitions(EntityTypeInterface $entity
   +    if ($published !== NULL) {
   +      @trigger_error('The $published parameter is deprecated since version 8.3.x and will be removed in 9.0.0.', E_USER_DEPRECATED);
   +      $value = (bool) $published;
   +    }
   

   +1. Love it! We should do trigger_error() with E_USER_DEPRECATED everywhere we are deprecating.

Thanks for reviewing :)

#88.1: Your comment is correct, but I'm not sure if we want to advertise the usage of NULL, that's why I didn't write it like that in the first place.
.2: Yeah.. me neither :)
.3: Both "...removed in 9.0.x" or "...removed before 9.0.0" are not actually true, this parameter will be removed exactly in 9.0.0 and it's also consistent with other 9.0.0 deprecation messages.
.4: Yup, we already do it a lot in other places.

but I'm not sure if we want to advertise the usage of NULL, that's why I didn't write it like that in the first place

Well, using NULL is better than a boolean :)

this parameter will be removed exactly in 9.0.0

I wonder how we'll do that? I still think it's not correct. You cannot deprecate exactly in 9.0.0, it will be deprecated during the dev cycle and that is before 9.0.0. "...removed in 9.0.x" or "...removed before 9.0.0" are the most used for the same reasons.

The failure seems unrelated.

Yes I think removed before 9.0.0 is most accurate - there'll be beta/rc releases before 9.0.0 is tagged etc.

Ok then :)

Hope we will not be visited anymore by random failures :)

+++ b/core/lib/Drupal/Core/Entity/EntityPublishedInterface.php
@@ -0,0 +1,37 @@
+   *
+   * @param bool|null $published
+   *   (optional and deprecated) TRUE to set this entity to published, FALSE to
+   *   set it to unpublished. Defaults to NULL. This parameter is deprecated in
+   *   Drupal 8.3.0 and will be removed before Drupal 9.0.0.
+   *

Patch looks great, and doing it this way actually looks a lot less bad than I thought it would when suggesting it :)

One question - should we explicitly point people to using setUnpublished() for unpublishing in the deprecation message or is it obvious enough?

Voilà!

I chatted with @catch about this yesterday and I also think this approach is a good one. Let's add a change record.

One question, how do we ensure that we actually do the removal for 9.0.0 when there is no @deprecated tag?

We cannot add a @deprectaed tag but we can add, at least, a @todo.

Draft change notice: https://www.drupal.org/node/2830201

@xjm on #102 we can grep for E_USER_DEPRECATED and remove all of those as well, also #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation should help with this.

Do we wanna open an issue against 9.x-dev for removing that, and add the url to the @todo?

#2716163: [META] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 9 branch is open already - to be honest I think that's plenty and we don't need the @todo. If we open 9.x and we haven't sorted out how to remove all deprecated method parameters by then, we shouldn't be opening it anyway.

Agreed with @catch that we can remove the @todo, we will be grepping the entire codebase for "deprecated" anyway when we start the clean-up process.

The change record looks good, thanks @claudiu.cristea!

If we're removing the @todo then the patch from #97 is what's needed. Here's a re-upload.

Updated issue summary ready for core committer review.

Committed d2af116 and pushed to 8.3.x. Thanks!

1. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -17,11 +18,21 @@
   -    $key = $entity_type->hasKey('status') ? $entity_type->getKey('status') : 'status';
   -    return [$key => BaseFieldDefinition::create('boolean')
   +    if (!is_subclass_of($entity_type->getClass(), EntityPublishedInterface::class)) {
   +      throw new UnsupportedEntityTypeDefinitionException('The entity type ' . $entity_type->id() . ' does not implement \Drupal\Core\Entity\EntityPublishedInterface.');
   +    }
   +    if (!$entity_type->hasKey('published')) {
   +      throw new UnsupportedEntityTypeDefinitionException('The entity type ' . $entity_type->id() . ' does not have a "published" entity key.');
   +    }
   +
   +    return [$entity_type->getKey('published') => BaseFieldDefinition::create('boolean')
   

   When I first saw this change I pondered why we weren't still supporting the status key in EntityPublishedTrait since it was before. But EntityPublishedTrait was added on 20 nov 2016 in #2810381: Add generic status field to ContentEntityBase so has never been official API because it is yet to be released. Therefore I think these changes are fine.

2. +++ b/core/lib/Drupal/Core/Entity/EntityPublishedTrait.php
   @@ -30,31 +41,37 @@ public static function publishedBaseFieldDefinitions(EntityTypeInterface $entity
   +      @trigger_error('The $published parameter is deprecated since version 8.3.x and will be removed in 9.0.0.', E_USER_DEPRECATED);
   

   Love it

3. +++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
   @@ -50,4 +50,22 @@ public function testCommentUpdate8101() {
   +  public function testPublishedEntityKey() {
   
   +++ b/core/modules/node/src/Tests/Update/NodeUpdateTest.php
   @@ -0,0 +1,41 @@
   +  public function testPublishedEntityKey() {
   

   Yay for upgrade path testing.

4. diff --git a/core/modules/comment/src/Entity/Comment.php b/core/modules/comment/src/Entity/Comment.php
   index 33b99c6..d52f04d 100644
   --- a/core/modules/comment/src/Entity/Comment.php
   +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -8,7 +8,6 @@
   use Drupal\comment\CommentInterface;
   use Drupal\Core\Entity\EntityChangedTrait;
   use Drupal\Core\Entity\EntityPublishedTrait;
   -use Drupal\Core\Entity\EntityPublishedInterface;
   use Drupal\Core\Entity\EntityStorageInterface;
   use Drupal\Core\Entity\EntityTypeInterface;
   use Drupal\Core\Field\BaseFieldDefinition;

Fixed unused use on commit.

I prematurely committed this. I missed the discussion about the rest change and @Wim Leers feedback. I agree with him that we should be addressing #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior first to prevent behaviour changes.

Setting to RTBC to mirror previous status but really I think it should be postponed.

So, here's an idea. I'm now sure how quickly #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior will happen and we have very long nested issue dependencies, anything we can get in and put behind us helps.

What if we special case the status key with a @todo to that issue above in rest.module? Then we do not cause a change here for comment* and we can do it properly without yet another change for comment*.

* Yes, for comment. Because here is the problem: The very argument that Wim made here to not commit this issue yet (it will cause a very-minor change for REST clients) is something that will happen with #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, on any not-really-read-only entity field.

And since node explicitly doesn't check for status being protected right now, adding a special case for the status key in general will in turn make the new node tests fail (we have no existing test for that for node I think). And if we make it comment specific, just to keep the tests happy, then any other entity that starts to use this will be hit that change twice like comment would.

@Berdir, that's a very good suggestion, it allows us to move forward here and leave #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior to handle the mess introduced in #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it.

This new patch does not modify any REST test case, so the argument from #64 that were blocking a commit is not valid anymore.

The code in EntityResource::patch() in HEAD is terrible, due to oversights/flaws in the Entity API.

--- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -213,6 +213,13 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
       // them. However, rather than throwing an error, we just ignore them as
       // long as their specified values match their current values.
       if (in_array($field_name, $entity_keys, TRUE)) {
+        // @todo Work around the wrong assumption that entity keys need special
+        // treatment, when only read-only fields need it.
+        // This will be fixed in https://www.drupal.org/node/2824851.
+        if ($entity->getEntityTypeId() == 'comment' && $field_name == 'status' && !$original_entity->get($field_name)->access('edit')) {
+          throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
+        }
+
         // Unchanged values for entity keys don't need access checking.
         if ($original_entity->get($field_name)->getValue() === $entity->get($field_name)->getValue()) {

The above hunk in this patch is making the terrible code in EntityResource::patch() more terrible.

That's IMHO a fine strategy to keep this unblocked!

The very argument that Wim made here to not commit this issue yet (it will cause a very-minor change for REST clients) is something that will happen with #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, hence results in incorrect behavior, on any not-really-read-only entity field.

No.

There's a very important difference.

This patch would introduce a behavior change. Then #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior would change it again. It's becoming very clear that fixing #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior will be very, very difficult. Because Entity API's semantics are not well-defined in those areas. So it's quite likely 8.3 would ship without #2824851. At which point 8.3 would have the behavior introduced by this patch, and then 8.4 could get the fixed behavior of #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.

What is repeatedly being forgotten here is that status is not and had never been a read-only field in the sense of "set and forget" (i.e. \Drupal\Core\TypedData\DataDefinitionInterface::isReadOnly() === TRUE. It's only read-only in the sense that only users with the administer comments permission are allowed to modify it. That's the first thing that \Drupal\comment\CommentAccessControlHandler::checkFieldAccess() does. So #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior will not cause the same REST test change as the one that was being made until #118 worked around it in a different way.

Random fail in OutsideInBlockFormTest, which we thought was fixed earlier today in #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly. Reopened that.

Somebody already queued this patch for re-testing, hopefully green again.

Let's trying committing again now that we have no unexpected behaviour changes.

Committed 67c7387 and pushed to 8.3.x. Thanks!

Small performance improvement followup: #2834291: Add a SQL index for entity types that are using EntityPublishedInterface

#2846830: Add changelog for Drupal 8.3.0

We forgot to publish the change record for this issue. Just did that now :)