Node form meta information should not come from a theme

Adding initial patch.

+++ b/core/modules/node/node.module
@@ -1367,3 +1367,46 @@ function node_comment_delete($comment) {
+function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {

This needs to be node_form_node_form_alter.

Sorry my bad, adding updated patch. Thanks timmillwood.

Should we have a test for this?

Moving to needs work for the tests.

This needs a reroll which is a novice task. See how to reroll a patch https://www.drupal.org/contributor-tasks/reroll

Reroll patch as per #10

Re-rolling again.

Added a simple test for these changes.

Adding a test only patch to show this actually fixes stuff.
Also re-uploading the patch from #15 just for clarity.

Looks good.

Moving this over to Cottser in case he wants to have a look first. Looks like a very minor addition to the node form to me, I'd definitely not have expected to find it in seven.

Yeah, this is certainly strange to be in Seven, but moving it now might have some tricky side effects?

When you use node forms without the admin theme, then it is quite common to have it be public for anon/authenticated users and this change could result in "things" being suddenly displayed, which affects user-visible parts of a website?

I see there's a screenshot above for stark, how does this look in bartik?

Also a ton of markup is apparently still seven specific and we're still changing all kinds of things there.

And also wondering about this combined with #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button.

CNR for at least the bartik screenshots.

From @Berdir in #20:

When you use node forms without the admin theme, then it is quite common to have it be public for anon/authenticated users and this change could result in "things" being suddenly displayed, which affects user-visible parts of a website?

Indeed that seems to be the most concerning potential side effect, in that it would be general user-facing.

We can potentially consider including this in 8.3.x but I'd like to run that by a release manager. We are short on time to squeeze it in for the first 8.3.x alpha.

Unassigning for now so we can get screenshots and discuss further.

Here's a re-roll of the patch and some screenshots in Bartik.

Drive by nit picks...

+++ b/core/modules/node/src/NodeForm.php
@@ -99,6 +99,42 @@ public function form(array $form, FormStateInterface $form_state) {
+    $form['meta'] = array(
+      '#type' => 'details',
+      '#group' => 'advanced',
+      '#weight' => -10,
+      '#title' => t('Status'),
+    );
+    $form['meta']['published'] = array(
+      '#type' => 'item',
+      '#markup' => $node->isPublished() ? t('Published') : t('Not published'),
+      '#access' => !$node->isNew(),
+    );
+    $changed = !$node->isNew() ? format_date($node->getChangedTime(), 'short') : t('Not saved yet');
+    $form['meta']['changed'] = array(
+      '#type' => 'item',
+      '#title' => t('Last saved'),
+      '#markup' => $changed,
+    );
+
+    // Add a revision_log field if the "Create new revision" option is checked,
+    // or if the current user has the ability to check that option.
+    $form['revision_information'] = array(
+      '#type' => 'details',
+      '#group' => 'advanced',
+      '#title' => t('Revision information'),
+      // Open by default when "Create new revision" is checked.
+      '#open' => $node->isNewRevision(),
+      '#attributes' => array(
+        'class' => array('node-form-revision-information'),
+      ),
+      '#attached' => array(
+        'library' => array('node/drupal.node'),
+      ),
+      '#weight' => 20,
+      '#optional' => TRUE,
+    );

Can we do short array syntax here please?

+++ b/core/themes/seven/seven.theme
@@ -157,32 +157,24 @@ function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {
+
+  $form['meta']['#attributes'] = array('class' => array('entity-meta__header'));
+  $form['meta']['#type'] = 'container';
+  $form['meta']['published']['#type'] = 'html_tag';
+  $form['meta']['published']['#tag'] = 'h3';
+  $form['meta']['published']['#value'] = $form['meta']['published']['#markup'];
+  unset($form['meta']['published']['#markup']);
+  $form['meta']['published']['#attributes'] = array(
+    'class' => 'entity-meta__title',
+  );
+  $form['meta']['changed']['#title'] = '<h4 class="label inline">' . t('Last saved') . '</h4>';
+  $form['meta']['changed']['#wrapper_attributes'] = array('class' => array('entity-meta__last-saved', 'container-inline'));
+  $form['meta']['author'] = array(
+    '#type' => 'item',
+    '#wrapper_attributes' => array('class' => array('author', 'container-inline')),
+    '#markup' => '<h4 class="label inline">' . t('Author') . '</h4> ' . $node->getOwner()->getUsername(),
+  );

And here as well :)

Thanks for the review @Manuel Garcia, here's the short syntax version of the patch.

Sorry more nitpicks...

+++ b/core/modules/node/src/NodeForm.php
@@ -99,6 +99,42 @@ public function form(array $form, FormStateInterface $form_state) {
+      '#title' => t('Status'),
...
+      '#markup' => $node->isPublished() ? t('Published') : t('Not published'),
...
+    $changed = !$node->isNew() ? format_date($node->getChangedTime(), 'short') : t('Not saved yet');
...
+      '#title' => t('Last saved'),
...
+      '#title' => t('Revision information'),

Let's do $this->t() instead.

Done.

Was tempted to update all, but out of scope, so just updated the ones added / edited by this patch.

Thanks, looks good to me now
@Cottser we've now got screenshots (see #24) ;-)

This doesn't really make it worse than it was before, but I'm still wondering if couldn't be a lot cleaner if we could move at least some of markup things in seven.theme into the node template. That code just feels "dirty" to me :)

While having a look at this, I thought that we could transition this to just being another details element, it looks good to me, although obviously not the same as we had before. I attach just an interdiff to illustrate what the change would need to be against the other patch and not derail the "patch conversation", assuming we want to keep the look we've got so far on Seven.

Apologies if this was already discussed back in the day =)

In any case, I agree we should clean up seven_form_node_form_alter to get rid of the markup there, but I think that we could do that as a follow up. We would be introducing a new template file that is not yet in Seven (node-edit-form.html.twig), and if we are going to do that we should aim to give other admin themes a good example on how to theme the node form - which deserves an issue to itself.

I quite like the suggestion in #31.

This issue is blocking us from altering the meta information in the sidebar in #2753717: Add select field to choose moderation state on entity forms.

The meta information is currently added in seven theme, therefore no modules can form_alter it, this issue would resolve that.

In content moderation we could have a state such as "archived" which sees the Node still being unpublished, just was a state labeled "archived". The issue here is the meta information still states "unpublished" which is confusing when the Content Moderation state is "archived".

I strongly feel the patch in #28 is a good solution, or at least a good first step in the right direction. Can we continue on with that, then open a follow up to discuss any further modifications such as #3?

And since #2882801: Review and improve the media creation form is also talking about having a sidebar for Media like Node already has … it looks like this is also blocking the Media Initiative now.

This needed a re-roll after #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button.

Also, the "dirty" code mentioned by @Berdir in #30 could be improved with the attached interdiff :) The screenshots from #24 (for Bartik) and the issue summary (for Seven) still apply.

Ah great, thanks @amateescu
Did a bit of manual checking, looks great to me, +1 to RTBC

In the re-roll in #24 we lost one of the additional tests that was in #16

--- a/core/modules/node/src/Tests/NodeCreationTest.php
+++ b/core/modules/node/src/Tests/NodeCreationTest.php
@@ -40,6 +40,7 @@ function testNodeCreation() {
     $node_type_storage->load('article')->delete();
     $this->drupalGet('node/add');
     $this->assertResponse(200);
+    $this->assertText('Not saved yet');
     $this->assertUrl('node/add/page');
     // Create a node.
     $edit = array();

It was not in the interdiff, so was it accidentally dropped, and should it go back in?

@jonathan1055, that's a very good observation! Yes, let's bring back that assertion :)

This does look a bit weird now in bartik though, because the status is duplicated fairly close together if you have administer nodes permission:

It also overlaps a bit with #2886569: Users with just create content permission don't know publishing status, especially on existing entities as you'd then get both the disabled checkbox and this new information?

I really don't want to be this annoying person who dislikes everything new (I feel very old right now ;)) but my comments on #2886569: Users with just create content permission don't know publishing status also apply here. If you have workflows where you use node forms to let users post stuff, without them even knowing that they are creating nodes/content, then this adds stuff to the page that wasn't there before. Maybe that's just me, but that's a pattern that we have used quite often in the past.

The same is true about the existing revision log field, but that was there before and you had to take care of it but this will then suddenly pop up on existing sites. If we're OK with that fine, but then we definitely need a change record that explains that.

If this issue lands and we do have a tab for 'status' which is viewable by all users, then am I right in thinking that #2886569: Users with just create content permission don't know publishing status will be redundant?

Also, to address your concern that the 'published' information is repeated, when this issue is done we could look at moving the new checkbox which was added in #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button to be inside this status tab, then all the status information would be in the one place, and the form for admins would be tidier with only the buttons showing at the bottom and no isolated checkbox.

Not reduntant, because this, just like seven in HEAD does *not* show the status for new content, just existing.

Also, about moving it, I'm not sure because we tried various things there, including having it in the vertical tabs but it is important that it is close to the submit button for UX. Also keep in mind that this looks completely different in seven, so the the checkbox would be in the sidebar. Which was actually something that I suggested in that other issue but others didn't like it (and I agree, was just an idea I had)

If you have workflows where you use node forms to let users post stuff, without them even knowing that they are creating nodes/content, then this adds stuff to the page that wasn't there before. Maybe that's just me, but that's a pattern that we have used quite often in the past.

I also did this a number of times, so I can totally relate to your comment :) How about restricting access to the 'meta' details element by default to uses that have the 'administer nodes' permission, and only show it by default for everyone in the Seven theme?

Briefly discussed the approach in #42 with @Berdir and he liked it, so here's a green patch, hopefully :)

+1 looks like a nice approach.

Of course, the test expectations have to be updated as well.

This is a blocker for #2753717: Add select field to choose moderation state on entity forms, marking as such.

I was talking to @Berdir in IRC about this issue and we couldn't find any reason to keep the 'author' meta info in the theme, so here's a new patch that finally moves all the meta elements to the node form itself.

The interaction between node form and seven is very complicated, especially as we start to try and expand/apply it to other entity forms as well. My comments in #2892304: Introduce footer region to ContentEntityForm have more examples of that.

I think this is a step in the right direction and unblocks a bunch of issues.

I opened two follow-ups for some ideas that @amateescu and I just discussed in IRC: #2893737: Move meta details element from NodeForm to ContentEntityForm and #2893740: Allow the sidebar for the node form to be used on other entity forms as well

This issue is also referenced by the media creation UX issue but it actually doesn't have any effect on that. #2893740: Allow the sidebar for the node form to be used on other entity forms as well on the other hand would.

Meant to do this.

The problem with moving the 'meta' form element from the theme form alter to the actual form is that they collide with the actual form elements with the same name ('author', 'changed', etc.), so we need to define the new 'meta' form element with '#tree' => TRUE in order stop that from happening.

Back to RTBC then.

+++ b/core/modules/node/src/NodeForm.php
@@ -102,6 +114,34 @@ public function form(array $form, FormStateInterface $form_state) {
+      '#access' => $this->currentUser->hasPermission('administer nodes'),

Wouldn't it make more sense here to use the 'view the administration theme' permission ? Then we should be 100% sure that the 'Status' fieldset will never appear in case

- seven is not enabled,
- or that user doesn't have permission to see admin themes
- a different admin theme is used

My concerns aren't really about showing it only in seven/the admin theme, keep in mind that the node forms also have a setting to use admin theme or not.

I think it is OK if we introduce new visual elements to adminstrative users or also editors, what I was worried about is showing weird fieldsets and so on in custom themes for anon/normal authenticated users.

Committed e0ea6c6 and pushed to 8.4.x. Thanks!

Awesome! This will make #2845425: Replace hook_form_node_form_alter() implementations with configured field layouts much easier.