Problem/Motivation
On #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button a footer region was introduced to NodeForm. It has surfaced while working on #2863431: Change "Save and keep un-/published" buttons for media module that this should be a more generic thing for all entity forms that implement EntityPublishedInterface so as to have consistency in the UI and avoid code duplication.
The only current use case in core where this would not be applicable is CommentForm. So double checking would be necessary.
Proposed resolution
Move the form footer region into ContentEntityForm for all entities implementing EntityPublishedInterface, remove it from NodeForm, and refactor the relevant CSS so it applies to all relevant forms.
Identify whether the need for a generic contententity-edit-form template would be necessary.
Remaining tasks
Do it.
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff.txt | 522 bytes | amateescu |
#27 | 2892304-27.patch | 1.68 KB | amateescu |
#22 | 2892304-22.patch | 1.64 KB | Manuel Garcia |
#7 | 2892304-7.patch | 1.64 KB | Manuel Garcia |
#7 | interdiff.txt | 1.83 KB | Manuel Garcia |
Comments
Comment #2
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedHere's a start.
Comment #3
BerdirThe only class that ContentEntityForm currently explicitly defines is 'class' => ['entity-content-form-revision-information'], should we be consistent with that and use entity-content-footer or so?
Comment #4
Gábor HojtsyThis would be amazing to support workflow's additions on the form.
Comment #5
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI don't think we should condition this region on
EntityPublishedInterface
, some workflows beside Content Moderation might not care about an entity being publishable or not.Comment #6
timmillwoodI agree with #3 and #5.
Also I wonder if it's worth pulling in
$form['status']['#group'] = 'footer';
in this issue.Comment #7
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks @all for the feedback & suggestions.
Addressing #3, #5 and #6
Comment #8
timmillwoodSorry, #6 was more of a question than an action. I'm on the fence about putting the status field in the footer generally, or do it on a per entity form basis. I guess even if we put status in the footer in ContentEntityForm, the entity still needs to update the base field definition to display the field in the form.
Comment #9
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented@timmillwood yeah I suppose leaving it on the node form makes sense because of that...
Could we perhaps update the base field definition on ContentEntityBase? Or perhaps EditorialContentEntityBase or EntityPublishedTrait?
Comment #10
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedActualy
ContentEntityBase
is used by all kinds of unrelated entities so thats a no go.EditorialContentEntityBase
however is only used by Node and Media at the moment...Comment #11
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedHow about updating the base status field definition in
EditorialContentEntityBase
?This should help cutting down #2863431: Change "Save and keep un-/published" buttons for media module and any other entity that follows this pattern...
Comment #12
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOr perhaps do this on
ContentEntityBase::baseFieldDefinitions()
?Comment #13
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedActually i already answered #12 on #10... so i think #11 is the way to go.
Comment #14
timmillwoodWe could update the base field settings on the original base field in \Drupal\Core\Entity\EntityPublishedTrait::publishedBaseFieldDefinitions?Comment entity also uses this trait, so we might have to override things there.
The entity key should be 'published'.
Comment #15
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRe #14:
I believe the key should be
status
. BothNode
andMediaType
define this key, but notComment
, so that serves as a way of signaling that the entity actually wants this field, so comments would not be affected by this change.Moving the change into
EntityPublishedTrait::publishedBaseFieldDefinitions
Node edit form looks exactly as before with this =)
Comment #16
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedArgh forgot to save the change on EditorialContentEntityBase before generating the patch... ignore the last one.
Comment #17
timmillwoodWe don't need to add this on, with the if hasKey, we can just update the original BaseFieldDefinition.
(and the key is published, not status. See Node, Media, and Comment.
Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI don't think we should change
EntityPublishedTrait
to show the field by default, that's for individual entity types to decide when and how they want it displayed.The patch in #7 was perfect IMO.
Comment #19
BerdirAgreed. This forces it on all entity types that implement this, it is up to them how to display it.
I'm still very confused about how exactly 9.x and deprecations and so on will work, but in theory, we could do what we did before.. introduce a setting to opt-in to this, and switch the default or remove it in 9.x, but I don't think this is worth it.
Also the CSS parts are tricky, because seven has quite a bit of node-form specific CSS, also e.g. the #edit-body thing above which is not limited to node form and is actually a configurable field, so different fields will look different. But if we suddenly apply all of that to all entity forms like comments and so on. And it's also partially tied to the node specific form alter that seven does (that we simplify but not completely remove in another issue) Maybe we could do a separte issue to introduce a something-form class that entity forms can add (or similar to above, we add by default in 9.x) to get the same styling for their forms.
Comment #20
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedI've done a bit of exploring, and providing a template for all editorial content entities I don't think is a good idea. The node form and the media form are very different in purpose and requirements so this would just get in the way, even with proper theme suggestions I think it could get very messy.
Re #17
Node
andMedia
define bothstatus
andpublished
keys, whileComment
only definespublished
, that's why I went withstatus
, but I could be missing something...Re #18 I can totally see the reasoning there. Hiding the other patches.
Comment #21
timmillwoodMedia doesn't have a
status
andpublished
entity key, it just haspublished
, but it looks like that's irrelevant if we go with the patch in #7. I would like to see an easier way to opt in or out, other than the changing the base field definition, but I guess @Berdir is right, it's not worth it.Comment #22
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRe #21 er... you're right, not sure what I was thinking, sorry for the nuisance!
Re-uploading #7 here so as to make it clear which way it's forward for this.
Comment #23
BerdirDetails elements have a useful #optional attribute, that does not show the wrapper if it is empty. That would be quite nice here as well because then we can guaranteee that the markup/output is not going to be altered unless you actually use that footer region.
Container unfortunately doesn't, and it is less likely to cause a visual change, but it could possibly still result in some additional spacing in theory.
Not sure if it's worth to add support for that for container or do a custom #pre_render for this element only here?
Comment #24
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedI think it would be great having
#optional
on Container, opened #2893586: Add #optional support to the Container render element =)Comment #26
tim.plunkettTracking this, since it's something we'll need to undo once we bring layouts to entity forms.
Comment #27
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#2893586: Add #optional support to the Container render element got in so let's use it here :)
Comment #28
BerdirLooks good.
Comment #30
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat was a random fail introduced by #2526150: Database cache bins allow unlimited growth: cache DB tables of gigabytes!.
Comment #31
larowlanDid some manual testing of this, looks good
Comment #32
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedw00t! great to see this all coming together :D
Re #26:
We could just add the layouts configuration to this region no? - otherwise, what's the plan? (I'm curious)
Comment #33
larowlanCan we get a change notice here selling this feature - thanks
Comment #34
timmillwoodAdded Change notice https://www.drupal.org/node/2908181
Comment #36
larowlanCommitted as 2e3db91 and pushed to 8.5.x.
Published change record.
Thanks.
Comment #38
tim.plunkettRetroactively removing tag as it didn't factor into the initiative after all