#1953528: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable introduced the status annotation property for config entities. It seems that this key is undocumented (albeit used for a few config entity types in core), but also that we can use this for some content entity types too, such as node.

Proposal

Split off the enable(), disable(), and status() methods into a separate interface that any entity type can optionally implement. We will then no longer need the status annotation key, but can use type hinting and calling status() instead. This removes an additional coupling between arbitrary code and entity types' implementations, and ensures we won't have redundant methods if entity types do not support statuses anyway.

API changes

None necessary, but that means we'll have to alias a few things:

  • The entity manager can populate the status property automatically by checking if the entity class implements the new interface.
  • We can alias NodeInterface's *Published() to the new methods.
CommentFileSizeAuthor
#3 interdiff.txt1.56 KBXano
#3 drupal_2129953_3.patch10.1 KBXano
#1 drupal_2129953_1.patch9.26 KBXano
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Status: Active » Needs review
FileSize
9.26 KB
Xano’s picture

Title: Extend status to content entities » Abstract entity status out to an interface
Xano’s picture

FileSize
10.1 KB
1.56 KB
Xano’s picture

Issue tags: +DX, +DX (Developer Experience)
Xano’s picture

Status: Needs review » Needs work

The last submitted patch, 1: drupal_2129953_1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 1: drupal_2129953_1.patch, failed testing.

dawehner’s picture

I think we should here at least make the CpnfigEntityBase to have a status. People will not look for a different base class which includes status.

Berdir’s picture

Quite sure that we already have an issue for this, I think @timplunkett started something.

What makes this i bit tricky is naming. Nodes also have a status for example, but it's meaning is completely different from config entities and even those often slightly differ in their meaning.

Xano’s picture

webchick’s picture

Can I ask where this impetus for fracturing EntityInterface into little tiny bits is coming from? First IdentityInterface, then Access(ible)Interface, now this.

IMO there's a lot more value in EntityInterface being a cohesive whole, so module developers only have to read one thing and get a good understanding of what's entailed in an entity, whether it's config or content.

Basically, what do we gain from this move? The issue summary is not clear.

webchick’s picture

That would also fit with nodes doing something a bit weird with their status field; they're merely subclassing and doing their own thing.

Xano’s picture

Can I ask where this impetus for fracturing EntityInterface into little tiny bits is coming from? First IdentityInterface, then Access(ible)Interface, now this.

AccessibleInterface and AccessInterface did and do not have anything to do with entities. AccessibleInterface is a generic interface that was namespaces as being for typed data only, and AccessInterface was for routing, but its namespace made it look like a generic interface.

The reason I want to split up the entity interfaces is that we have put all kinds of functionality on them, but that not all entities use it. Technically the entities conform to the contract, but they don't actually do anything with it. In case of contact_message entities, calling $entity->save() will actually break, because the method implementation assumes a storage controller, which is never configured for good reason. However, as EntityInterface requires entities to implement CRUD methods, there is no way of knowing whether such entities can be saved at all without checking the annotation. As such, I would argue this is a broken implementation of a contract.
On the other hand we put things on ConfigEntityInterface that may not be limited to config entities, such as statuses. Most config entities use it, but not all, such as taxonomy_vocabulary, which is a broken contract implementation. On the other hand some content entities could benefit from it as well, such as node.

damiankloip’s picture

Hmm, I'm not sure about this to be honest, you can't split up interfaces for every bit of functionality. Otherwise it become a nightmare to actually implement.

As it currently stands, me implementing a status key in the entity type annotation is very easy. I don't have to know much else. For things like this having a null op is acceptable imo (?). E.g. in the cache MemoryBackend, it doesn't make sense to do anything with removeBin(), so it's just a null op, empty method. Otherwise, we would have some other interface that goes on top of this again? Not saying we will do that, but you get the idea.

Also, for statuses with config entities, this is really all up to how the calling/implementing code deals with it anyway. Nowhere in the config entity classes do we do anything with checking the status of an entity.

Xano’s picture

So nobody agrees with me that it's just downright idiotic that we force entities to pretend they offer particular functionality, while they actually don't? Having enable(), disable(), status(), and setStatus() methods implies an entity type has a status and it can be changed. Code working with entities will have to check the entity key from the definition, which IMHO is more an implementation detail that anything else and also requires more code than simply checking whether an entity implements a particular interface.

damiankloip’s picture

What code should be checking entity statuses for example? Status should really only be used by the module implementing the entity, as this can differ quite alot per implementation. So there shouldn't really be much generic code ever checking this. See the issue where status for config entities was done for all the points on that. They also default to enabled, if you want to think of it like that.

So you would have to specify this in entity info and implement the correct interfaces?

tim.plunkett’s picture

Issue tags: -DX
Xano’s picture

What code should be checking entity statuses for example?

Any code that needs to use config entities in the UI, and therefore may need to know if they are enabled and are available for use. I maintain the Currency module and someone might want to make a custom selector that should only include the ones that are enabled. According to your argument, they should not use the entity status, because they're not coding on the module that owns the entity type.

If you agree on this, one can also argue that if you work with a specific entity type anyway, you know whether statuses are supported or not. However, this is not true, as we type hint on interfaces and use that to know which methods are available to us. If an entity without statuses passes the ConfigEntityInterface type hint, and we call status() on it, we get failures or at the very least unexpected results.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

Status: Needs work » Closed (duplicate)