Problem/Motivation
In #2810381: Add generic status field to ContentEntityBase we're looking to make the status field, used for published/unpublished status on nodes and comments more generic. Although there is currently no generic interface for this.
Proposed resolution
Introduce EntityPublishedInterface and name use of it in NodeInterface and CommentInterface
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 2811667-21.patch | 8.42 KB | martin107 |
| #21 | interdiff-2811667-18-21.txt | 786 bytes | martin107 |
Comments
Comment #2
timmillwoodInitial patch adding the EntityPublishedInterface.
Note: #2009958: Introduce EntityStatusInterface to standardize how entities can be disabled adds a very similar interface but focuses on config entities (enabled/disabled state), where as this focuses on content entities (published/unpublished state).
Comment #4
yogeshmpawarComment #5
timmillwoodFixing patch
Comment #7
timmillwoodfixing bug introduced in #5
Comment #9
timmillwoodActually tested it this time.
Comment #11
timmillwoodFingers crossed
Comment #13
timmillwoodForgot to update Node.
Comment #14
phenaproxima+1 to this idea. I have a question, though: why are the STATUS_PUBLISHED and STATUS_UNPUBLISHED constants in ContentEntityBase? Would it maybe make more sense for them to be part of EntityPublishedInterface and renamed PUBLISHED and UNPUBLISHED?
Comment #15
Anonymous (not verified) commentedThis is a great idea, however the issue I have found that needed to be solved in the past is actually tracking the date that the item was published, which is separate to the date the item was created. Solutions currently bring the created date forward, which is semantically incorrect. This is particularly relevant when listing content in order of publication date.
Would it be possible to discuss adding this property to the node entity as part of this feature? It would be recorded on the first transition change from unpublished to published, or be identical to the created date if published immediately. Items that are then unpublished will retain this date, however would be required to bring it forward if necessary. As a result of this, the created date should then be ideally be made immutable.
Comment #16
timmillwood@GroovyCarrot - This is a great idea (but a separate issue).
Comment #17
Anonymous (not verified) commentedMakes sense, in which case should probably be deferred until after this is merged in.
@phenaproxima there's probably no value in adding constants to represent TRUE and FALSE for published status at the interface level. It does make sense to have the status constants if there are multiple statuses, however since there is only 2 statuses (published and not) there is no immediate added value. I guess it does accommodate for something like a 'needs review' status, which is still unpublished.
Off the back of this, if published status for an entity is a result of a series of factors (status, created date in the past, etc), then perhaps
EntityPublishedInterface::setPublished()should be removed, and the entity allowed to decide what it considers to mean published - such as a simple underlying status property. If a service requires the ability to set an entity to 'published' state, then it should be segregated into a separate interface (EntityPublishableInterface) for readability, and to be explicit in that the service intends to mutate that state.Comment #18
timmillwoodI agree with #14, the constants should be in EntityPublishedInterface (they were in an earlier patch, but moved them during debugging).
We can't however called the constant PUBLISHED because CommentInterface as a constant PUBLISHED and if CommentInterface implements EntityPublishedInterface we can an override error (as seen in some earlier failing tests).
I'm not sure I understand the final paragraph in #17.
Comment #19
ebeyrent commentedDo we need to declare these constants again? It looks like this patch removes the use of PUBLISHED and NOT_PUBLISHED in favor of the constants declared in EntityPublishedInterface, which I think is the better way to go. Can we simply remove these?
Comment #20
timmillwoodWe can't remove them for backwards compatibility.
Comment #21
martin107 commentedFixing ultra trivial coding standard fixes to our shiny new EntityPublishedInterface, move along nothing to see here :)
Comment #22
Anonymous (not verified) commentedConstants here are unnecessary as they are meaningless to the interface; the reason for this is because
isPublished()returnsbool, and not one ofSTATUS_PUBLISHED|STATUS_UNPUBLISHED. Using status constants would overcomplicate this, since there's nothing wrong with TRUE or FALSE regarding whether or not something is in a published state.The interface is an abstraction for a client, and shouldn't dictate how the object implements the method. To elaborate this a bit more: the node entity has chosen to use a
statusproperty to track whether or not it is published, at the moment this is either published (1), or unpublished (0).isPublished()returns(bool) $this->getEntityKey('status'), so the method still conforms to the interface and returns a boolean, without you needing to care what the underlying state codes mean. This allows them to be changed by the entity at any point in future (so you don't have to worry about BC breaks).Just because an entity has a recognised published state, does not necessarily mean that it can, or should be toggled. An interface should not require that the state can be changed / is mutable by the API, unless that is an explicit requirement by the client; in which case it should require a different interface that allows that.
For example, the following interfaces are more explicit, and are better suited to what the client would expect to do with the entity.
Comment #23
timmillwoodI'm not sure I agree with the comments in #22.
The plan for this patch is to give a common interface for both node and comment entities, and possibly others in the future. We are not looking to redesign the platform, just abstract the common functionality and remove duplication.
Comment #24
Anonymous (not verified) commentedI'd argue that #13 was right then, with the implementation constants on the abstract class; however I'd still suggest segregating the interface better so that it's more specific towards the client, and there's room for improvement.
Comment #25
timmillwood@GroovyCarrot - Because of the way constants have been used in node.module and CommentInterface historically I think these should be moved to EntityPublishedInterface and not ContentEntityBase. I don't think we can or should have entity types with a published/unpublished status with anything different than 1/0, it should be consistent.
Comment #26
Anonymous (not verified) commentedThe interface just doesn't make sense on it's own with the constants that are integers, and methods that return or accept a boolean. The behaviour you are describing is better placed with a trait if it is to direct how common functionality is implemented?
Comment #28
timmillwoodComment #29
amateescu commented@timmillwood, isn't this a duplicate of #2789315: Create EntityPublishedInterface and use for Node and Comment? The initial patch over there introduces
EntityPublishedInterfacebut in comments #3 and #10 you advocated forEntityStatusInterfaceinstead.. it's a bit confusing to have so many duplicate issues around this :/Comment #30
timmillwoodAfter discussions in #2810381: Add generic status field to ContentEntityBase I'm not sure this needs to specifically focus on the 'status' entity key.
Comment #31
timmillwoodClosing in favor of the original issue #2789315: Create EntityPublishedInterface and use for Node and Comment.