Add a content entity form which allows to set revisions

+use Drupal\entity\Entity\RevisionableEntityBundleInterface;

Old namespace.
We need to make sure patches are clear of \Drupal\entity

So this patch can't work without #2666808: Add a revisionable entity base class and log interface landing. Might be worth merging into 1 issue?

Addressed the silly mistakes.

There's an existing related issue for this. Searching...... here we go: #2329939: Unifying the revision checkbox in entity forms

That changes the default class. I'm wondering if we can do that in a minor release. This patch does quite a bit more as it also adds a save() method, that I guess is not something we can do.

This patch adds new code (a new base class for interested entity types), doesn't convert any old code.
So I don't see how the BC argument applies.

I know it does, but that means it also has the same problem as all base classes. You can use this and SomeOtherHelpfulEntityFormBase class combined. It also needs to be documented and developers need to explicitly use it. Would be nicer if it *would* be part of ContentEntityForm.

But I don't know if we can do *that* :)

+++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
@@ -0,0 +1,156 @@
+    $form['advanced'] = [
...
+    $form['revision_information'] = [

why not reuse for node form?

maybe there's other way to re-use the code? maybe a trait somehow...

1. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,156 @@
   +    if ($bundle_entity instanceof RevisionableEntityBundleInterface) {
   +      // Always use the default revision setting.
   +      $this->entity->setNewRevision($bundle_entity && $bundle_entity->shouldCreateNewRevision());
   +    }
   
   +++ b/core/lib/Drupal/Core/Entity/RevisionableEntityBundleInterface.php
   @@ -0,0 +1,22 @@
   +interface RevisionableEntityBundleInterface extends ConfigEntityInterface {
   

   Nothing in this patch implements this new interface. Which means the quoted code path cannot have test coverage.

2. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,156 @@
   +    // @todo Could we autogenerate this form by using some widget on the
   +    //   revision info field.
   

   That seems out of scope here? E.g. NodeForm also doesn't do this, and also should. Separate issue?

3. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,156 @@
   +      '#access' => $this->entity->isNewRevision() || $account->hasPermission($entity_type->get('admin_permission')),
   ...
   +      '#access' => $account->hasPermission($entity_type->get('admin_permission')),
   

   Fails to pass on cacheability metadata. I know this is a form and will likely never be cached, but it eventually might. Better to do things right going forward, no?

4. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,156 @@
   +      '#default_value' => $this->entity->getRevisionLogMessage(),
   

   This should only be prefilled when editing an existing revision?

5. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,156 @@
   +      if ($this->entity->getEntityType()->hasLinkTemplate('collection')) {
   +        $form_state->setRedirectUrl($this->entity->toUrl('collection'));
   +      }
   +      else {
   +        $form_state->setRedirectUrl($this->entity->toUrl('canonical'));
   +      }
   

   One line of documentation explaining the difference might be nice.

This is a blocker for media_entity in core.

For fun, trying to use it for node form. We can still not do that here but IMHO, it makes sense to see how the result looks like. Might also do the same for BlockContent. There are quite some differences. I changed a few but not all yet.

* node form uses a widget for revision log, that makes sense to me and RevisionLogEntityTrait does so too, so removed that from the form.
* node form uses top level form elements and #group, changed that
* permission are tricky, the base currently hardcodes that there is an admin_permission, node does not and will not have that
* Tried to make getBundleEntity() more generic and reliable by making sure we actually have a bundle entity type and load through entity storage which seems easier to me as we have that info anyway and we also have the entity type manager anyway.
* node hides the revision checkbox for new entities, that is a recent change that makes sense to me

Some remaining differences:
* node has some special classes (I did move one, entity-meta, which happens to have a very generic name anyway)
* As mentioned, node has slightly different access logic
* inversed logic for the revision checkbox and log message. node shows the message if the checkbox is checked, this checks the box if you write something. hiding the message box imho makes more sense to me, although the other one is a bit faster if you know what you're doing I guess

1. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,156 @@
   +    // Save as a new revision if requested to do so.
   +    if (!$form_state->isValueEmpty('revision')) {
   +      $this->entity->setNewRevision();
   +    }
   

   since we set a default value, I'm wondering if this really works if the default is enabled and you uncheck?

2. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,156 @@
   +    $logger = $this->logger($this->entity->id());
   

   that should be the entity type id, not the id ;)

   Is this really like this in entity.module? :)

3. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,156 @@
   +    $t_args = ['@type' => $bundle_entity ? $bundle_entity->label() : 'None', '%info' => $this->entity->label()];
   

   "None doesn't make sense, should use the entity type albel :)

4. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,156 @@
   +    else {
   +      // In the unlikely case something went wrong on save, the entity will be
   +      // rebuilt and entity form redisplayed.
   +      drupal_set_message($this->t('The entity could not be saved.'), 'error');
   +      $form_state->setRebuild();
   +    }
   

   this stuff is nonsense :)

   if something indeed goes wrong, then save() throws an exception. pretty sure this is copied from node form, but this is dead code there too.

Some obvious bugfixes and a not-so obvious one. setNewRevision() is a one-way street. Once called, you can't not not saved as a new revision anymore. It simply silently ignores that. Which I guess is a bug, but that we can only solve after #2809227: Store revision id in originalRevisionId property to use after revision id is updated is in, as we right now have no way to restore the revision ID.

Ah, I see now. The differences to node form are because this is copied from BlockContentForm. Which means it a) works quite differently to the node form and has a bunch of those bugs that I identified, like it is not actually possible to not save a new revision and we have no test coverage for that :)

On the plus side, it means that most of that form can go away now, including almost all of the save() method.

More test fixes. Restored the current node edit title, we could also make that the default.

1. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,150 @@
   +  protected function prepareEntity() {
   +    parent::prepareEntity();
   +
   +    // Set up default values, if required.
   +    if (!$this->entity->isNew()) {
   +      $this->entity->setRevisionLogMessage(NULL);
   

   Why log message not having this as default value?

2. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,150 @@
   +  protected function getBundleEntity() {
   ...
   +    return NULL;
   

   this is default to return null, why that needed?

3. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,150 @@
   +    // @todo Could we autogenerate this form by using some widget on the
   +    //   revision info field.
   +    $form['revision_information'] = [
   

   this needs follow-up or bullet in summary

1. Because this is not a default value for new entities, it is to replace the message of the currently saved revision, you don't want to see that in the UI.
2. It is not needed.
3. I don't think we can do this as a widget, so I think we should just remove that todo.

Great work. Thanks!

1. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,150 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Entity\Form\RevisionableContentEntityForm.
   + */
   

   Can be removed. Applies to all files.

2. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,150 @@
   +    // @todo Could we autogenerate this form by using some widget on the
   +    //   revision info field.
   

   Remove as mentioned above.

3. +++ b/core/modules/node/src/NodeForm.php
   @@ -4,6 +4,7 @@
    use Drupal\Core\Entity\ContentEntityForm;
   

   Should be unused at this point I think.

   Also noticed that constructor wrongly refers to it in its comment. Maybe we can fix that along the way?

4. +++ b/core/modules/node/src/NodeForm.php
   @@ -110,38 +94,16 @@ public function form(array $form, FormStateInterface $form_state) {
   +      $form['#title'] = $this->t('<em>Edit @type</em> @title', array('@type' => node_get_type_label($node), '@title' => $node->label()));
   

   Would it make sense to use $node->type->entity->label() since we're already touching this?

:)

About 4. We could also make the new class return the same structure as NodeForm does, then we can remove it from there. We need to improve that a bit anyway and have separate texts for bundle/no bundle. And we should use the bundle info service to get the bundle label also if the entity doesn't use config entities for bundles. That might result in a weird label for entity_test, though.

Worked on comments from #34

1. Fixed in RevisionableEntityBundleInterface and RevisionableContentEntityForm
2. Removed @todo as suggested by @berdir
3. Fixed
4. I think so. Get rid of global functions is always nice

I also fixed the constructor comment and some other coding style issues

Beside that i checked the patch and had an issue with the revision_log field, which was not in the advanced group.

Fixed that, too.

1. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,140 @@
   +    // Set up default values, if required.
   +    if (!$this->entity->isNew()) {
   +      $this->entity->setRevisionLogMessage(NULL);
   +    }
   

   Let's explain why are we doing this in a comment. I don't think that it will be obvious to everyone.

2. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,140 @@
   +      '#attributes' => array('class' => array('entity-meta')),
   
   +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -166,55 +47,21 @@ public function form(array $form, FormStateInterface $form_state) {
   +        array(
   +          'plugin_id' => 'block_content:' . $block->uuid(),
   +          'theme' => $theme,
   +        )
   

   Let's use short array syntax consistently.

3. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -106,10 +100,6 @@
   -    if (isset($form['revision_log'])) {
   -      $form['revision_log']['#group'] = 'revision_information';
   -    }
   -
   

   This change caused revision log textarea to be outside of the vertical tabs on custom block form. See screenshot:

   

   I also think that it is strange that we create vertical tabs in base class and put everything except one element in them (and hand over responsibility for it to child classes).

Addressed everything from #40

Also i moved the structure from the #title property into the RevisionableContentEntityForm.

Checked node and custom block form, both are looking well now.

1. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -47,23 +47,26 @@
        if ($this->operation == 'edit') {
   -      $form['#title'] = $this->t('Edit %bundle_label @label', [
   -        '%bundle_label' => $bundle_entity ? $bundle_entity->label() : '',
   +      $form['#title'] = $this->t('<em>Edit @bundle_label</em> @label', [
   +        '@bundle_label' => $bundle_entity ? $bundle_entity->label() : '',
            '@label' => $this->entity->label(),
          ]);
        }
   

   as discussed in IRC, we should use the bundle info instead.

   Also, I would recommend to use an if ($bundle_label) { } else { } here, with different texts. This will have double space otherwise I think?

2. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,147 @@
   +
   +    // Check the revision log checkbox when the log textarea is filled in.
   +    // This must not happen if "Create new revision" is enabled by default,
   +    // since the state would auto-disable the checkbox otherwise.
   +    if (!$this->entity->isNewRevision()) {
   

   this check is now wrong and never TRUE.

   As mentioned before, this is different to node form, and what node form does IMHO makes more sense (show message field if checkbox is checked). this is checking the box if you start writing in the field. I think what node does makes more sense. This would also allow you to set a message without saving a new revision. No idea what that would even do.

3. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -44,121 +19,27 @@ class BlockContentForm extends ContentEntityForm {
   -
        // Add a 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',
   

   seems like we can remove this comment?

4. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -44,121 +19,27 @@ class BlockContentForm extends ContentEntityForm {
   +    $form['revision_information']['#attributes']['class'][] = 'block-content-form-revision-information';
   +    $form['revision_information']['#attached']['library'][] = 'block_content/drupal.block_content';
   

   both node and block_content add their own js here. Node has some additional things with promote and so on, but most of the file is actually the same (translation and revision features). I'm wondering if that's something we can generalize so that this would work out of the box?

   And I think the media_entity patch is adding another version of this right now too.

5. +++ b/core/modules/node/src/Entity/NodeType.php
   @@ -205,4 +205,11 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function shouldCreateNewRevision() {
   +    return $this->isNewRevision();
   

   BlockContentTypeInterface has this too, lets make sure it extends from the new interface so that this works (apparently no test coverage for this, like many other revision related things in the block_content UI)

6. +++ b/core/modules/node/src/NodeForm.php
   @@ -110,47 +93,22 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['advanced']['#attributes']['class'][] = 'entity-meta';
   

   I think we add this in the parent now, or not?

7. +++ b/core/modules/node/src/NodeForm.php
   @@ -110,47 +93,22 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['revision_log'] += [
   +      '#states' => [
   +        'visible' => [
   +          ':input[name="revision"]' => ['checked' => TRUE],
   +        ],
   +      ],
   +    ];
   

   this. My suggestion would be to move this up in the base class and replace what we have there with this.

1. Fixed, injected bundle info service and use it
3. Removed comment
4. Discussed that with @tstoeckler and we agree. We should introduce a misc/entity.js and should node.js and block_content.js depend on that
6. Add this class to block_content looks not very nice

7. Same here, move that into the base class, looks not very nice for block form

I will work on 2, 4 and 5 tomorrow.

Forgot the patch..

I am on it, new patch will follow

@tim.millwood just brought up #2696555: On the entity form of revisionable entities, make "create new revision" and "revision log" configurable in IRC, which - to me at least - is a pretty great idea. So I think we should at least consider to completely leave out the revision checkbox and always create a new revision.

I think there is quite a lot of overlap with the workflow initiative so tagging it so we can track too.

I'm not 100% convinced yet. I think there are sites that might want to give users a choice. Especially with translations and so on, there are IMHO valid use cases to avoid saving new revisions.

From that issue:

The decision to save new revisions or not is a site building or developer issue, not a content author one.

Maybe, but why don't we let the site builder/developer decide what they want to allow?

Note that at least for nodes, you already need administrative permissions to change the default (which means you can also publish/unpublish, change author, ...), so this permission already exists to allow them that kind of control. But it is global. We could add another setting to the node type to control if the setting should be enforced or not?

So here is a new patch. Worked on the comments from #43

2. For me its TRUE if i check new revisions on admin/structure/block/block-content/manage/basic

4. I created an entity.js and moved the duplicated code there

5. BlockContentTypeInterface now extends RevisionableEntityBundleInterface

Here is the next update:

1. Renamed it to drupal/entity-form
2. Fixed and removed EntityRevisionLogInterface, because it does not exists
3. Nothing changed, see #44 - 6
4. Fixed
5. Checks the update access of the revision field now
6. $form['revision'] is inside the details $form['revision_information'] which is inside the vertical_tabs $form['advanced']
7. Nothing to do :)
8. Fixed

Great progress, we are almost there. Noticed just a few minor things:

1. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,199 @@
   +        $bundleLabel = $bundle_info[$this->entity->bundle()]['label'];
   +        $form['#title'] = $this->t('<em>Edit @bundle_label</em> @label', [
   +          '@bundle_label' => $bundleLabel,
   

   $bundleInfo is used only once. I guess we can remove it and assign value directly.

2. +++ b/core/modules/node/src/Entity/NodeType.php
   @@ -205,4 +205,11 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +    return $this->isNewRevision();
   

   Should we deprecate isNewRevision()? It is now replaced with shouldCreateNewRevision(), which I assume will become a standard.

3. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,199 @@
   +    $account = $this->currentUser();
   

   This variable is unused.

4. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,199 @@
   +    // Check the revision log checkbox when the log textarea is filled in.
   +    // This must not happen if "Create new revision" is enabled by default,
   +    // since the state would auto-disable the checkbox otherwise.
   +    if (!$this->entity->isNewRevision()) {
   +      $form['revision']['#states'] = [
   +        'checked' => [
   +          'textarea[name="revision_log[0][value]"]' => ['empty' => FALSE],
   +        ],
   +      ];
   +    }
   

   Condition should be !$new_revision_default. Custom block form won't respect the bundle configuration if revisions are disabled (reason explained in comment).

+++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
@@ -0,0 +1,199 @@
+    // Save as a new revision if requested to do so.
+    if (!$form_state->isValueEmpty('revision')) {
+      $this->entity->setNewRevision();
+    }

Noticed another thing while working on #2831274: Bring Media entity module to core as Media module. We are setting new revision here but not uid and timestamp. As a result of that all revisions end up using same values for those two fields for every revision. This is reproducable with block but not with node as it has some custom code that does that (submitForm()). I think that we need to set uid here, while I'd rather put timestamp in preSaveRevision(). That way we ensure that we're setting new timestamp even when an entity is saved programatically.

Here are changes for comments in #55 and #56

#55:
1. I think you mean $bundleLabel. Have fixed that
2. Should be addressed in an follow-up
3. Fixed
4. Fixed

#56

I moved the submit function from NodeForm into RevisionableContentEntityForm, because it seems that logic should be the same. Also i added some logic in BlockContent and Node preSaveRevision to set the correct timestamp. I'am not 100% sure of this solution.

1. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -164,6 +162,31 @@
   +    // Save as a new revision if requested to do so.
   +    if (!$form_state->isValueEmpty('revision') && $form_state->getValue('revision') != FALSE) {
   +      $node->setNewRevision();
   +      // If a new revision is created, save the current user as revision author.
   +      $node->setRevisionUserId(\Drupal::currentUser()->id());
   +    }
   

   If we do this here we don't need it on save() as well. Condition could also be simplified (similar as there) too.

2. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -135,6 +135,9 @@
   +    else {
   +      $this->setRevisionCreationTime(REQUEST_TIME);
   +    }
   
   +++ b/core/modules/node/src/Entity/Node.php
   @@ -123,6 +123,9 @@ public function preSaveRevision(EntityStorageInterface $storage, \stdClass $reco
   +    else {
   +      $record->revision_timestamp = REQUEST_TIME;
   +    }
   

   I was thinking about adding this to base entity class.

EDIT: fix typo

1. Fixed
2. Moved both calls back into submitForm, because it breaks lots of test. Maybe we can refactor this when #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table lands

Beside that, i added the RevisionLogEntityTrait to EntityTestRev to fix broken tests

Fixing the tests...

Just checking: does this introduce new user interface elements somewhere?

@yoroy: Not really, with the exception of one detail that we don't fully agree on. Will come back to that in a second.

What this is does is unify the code in custom blocks and nodes for the revision related UI parts.. specifically the checkbox for creating new revisions and the revision log message. Both forms should continue to work as they do right now.

There is one small thing that I'm not sure about. The node form currently has a checkbox "[ ] Create new revision". If you check that, then the message textare becomes visible and you can enter a message.

Currently, the new base class that provides the default implementation for all upcoming forms that want to provide a UI to save new revision provides a slightly different user experience. Both the checkbox and the textare are by default visible. And when you start to enter something in the log, then it checks "Create new revision". This is actually the behavior used by custom blocks. Why they are different at the moment I don't know. Custom blocks are also pretty broken right now.

To me, that seems to be targeted towards experienced users, it allows them to save a click as they can just type something in the log. But I feel like this makes it harder to understand for new users and I think most users will click the checkbox first anyway. As that is the first decision that you make.

Note:
* Node form is currently not changed and works like in HEAD
* Custom block forms and node forms look very different (custom block uses vertical tabs and node uses the sidebar thingy). Using the node behavior on custom blocks looks a bit strange as it results in a very slim vertical tab
* For nodes, the checkbox is only visible for users with "administer nodes"
* This only really makes a difference when new revisions are not enabled by default (We now have them enabled by default for new installations)

So the question/decision to make is:
* What should the default implementation do, node or custom block like behavior (Current patch: custom block)
* Should we update the other form to use the new default (Current patch: existing node form is unchanged)

Also, there is a second thing. See #7-10.

So we add this new base class. But there is also #2825973: Introduce a EditorialContentEntityBase class for revisionable and publishable entity types. If that ends up being another base class. Then you can't have both, unless one extends the other. I'm actually not sure what the exact difference between the two issues is. Maybe the should be the same class, maybe not.

Lets say they are not (If not those two then there might be another form feature). What is a content entity supposed to do then?

I misread that other issue. That is not a form base class, it is an entity base class. That said, this is still true in principle

#10:

Well yeah in that case we would need some kind of annotation/flag/method call to add the revision form. At that point, I don't see much of a difference to the dedicated form class to be honest, but yeah its sad that we cannot go back in time, let's say 2 years ago ...

I see two major differences/advantages:

* it is much easier to combine multiple flags in ContentEntityForm than it is to combine multiple base classes.
* Such a flag is something we can deprecate for 9.x where we will simply always do it, or at least set it to enabled by default.

Re #65: I think #44 explains why there is different behavior. The node form has the custom sidebar where a group/tab/details with a single checkbox actually looks OK. For custom blocks there are the standard vertical tabs and those look weird if there's just a checkbox and nothing else.

That's true. But even I (with my noob CSS skills) could improve the custom block look with a min-height.

Also, the sidebar thing is hardcoded in seven.theme. You don't get that if you use the frontend theme for the node edit form or a different backend theme. And I'm wondering if we should expand the seven UI to other entity types too, don't really see why we would want to have different UI's for nodes vs. custom blocks vs. media entities vs ..

I think its best to not get into debating changing existing UIs. Also the block form UI makes more sense to reproduce by default IMHO given that most entity forms are not multi-column like nodes. Also it helps the chances of this patch greatly to not open up UI questions like that ;)

Re #69 / @Gábor Hojtsy: I think the point was that ideally we would want the form class introduced here used by both nodes and custom blocks as those are the only incarnations of revisionable entities in core as of now. But doing that without providing a consistent UI requires workarounds in at least one of the forms. So because NodeForm is already weird in a couple of ways, I personally would be fine adding a tiny bit more weirdness to it which the latest patch is doing, but I do think it's valid to discuss here. And if in fact the UX-end-goal would be for everything to be like the node form that it might actually make sense to make that the default now, which I think is what @Berdir was hinting at...

First: lets not change the node form in this issue :-)

Before we had the sidebar on node screens these items were shown in vertical tabs there as well. The single checkbox is not per se the problem. The single checkbox stands out because it gets its own section (on node screen) or own vertical tab. The same pattern is used for creating a menu link for a node. There's a single checkbox for it. You check it first, then the other bits of UI are revealed.

Also considering:

This only really makes a difference when new revisions are not enabled by default (We now have them enabled by default for new installations)

Then I think it would be best to follow the current pattern of a single checkbox first like on node forms and apply that here as well.

Talked a bit more with @yoroy in IRC about what he meant with that exactly:

1. The default should be what NodeForm does
2. NodeForm shouldn't change then (there is nothing to change to now anyway)
3. custom block should work like node form
4. No special CSS for now (like a min-width to increase the height of that element)

If others are unsure/unconvinced about 3 & 4, then we could also keep the current behavior in BlockContentForm and open a follow-up to discuss that?

3. custom block should work like node form

That seems like a pretty non-trivial undertaking, so certainly not in the scope of this issue, unless I'm missing something. There's already a number of issues for making the node styling more generic (none of which I can find right now... :-/). So I'm not really sure what #72 is proposing as for the scope of this issue. Can you elaborate?

Sorry. 3 *only* applies to the behavior of the #states stuff for the checkbox + textarea. Nothing else.

But again, I think making the default work like node makes sense, whether or not we update custom block here isn't that important. We could keep its own #states logic there and look at removing that later. We would likely need to explicitly replace it, though, not just append more.

Ahh, Ok. That makes Sense, Thank you!

Here is a new patch, that addresses all the comments from @berdir in #72

Patch from #77 works as expected. The custom block form now acts the same as the node form. Unless changing the UX of the custom block form is a problem that should be handled in a different issue, I think it is RTBTC!

+++ b/core/modules/node/src/NodeForm.php
@@ -65,7 +65,7 @@
 
     if ($preview = $store->get($uuid)) {
-      /** @var $preview \Drupal\Core\Form\FormStateInterface */
+      /** @var \Drupal\Core\Form\FormStateInterface $preview */

This might conflict with another RTBC issue: #2548713: Only one additional new value saved unlimited field and no non-field values are restored after preview

Might conflict anyway but can we maybe keep this unrelated change out of this issue?

I think the last thing to decide is #66. In case it is not clear what I mean there, a quick example how that could look like:

We introduce a new annotation flag, something like show_revision_ui, default to false (maybe switch that to true in 9.x) and then move that code directly into ContentEntityForm, and wrap the necessary parts into a if ($this->entity->getEntityType()->get('....').

Why I think this would make sense:

We now add the advanced settings vertical tab in our method. If we want to add more default UI's in there in another base class/trait, this might complicated. If we have it in a base class, we could for example do if (this_feature_check || another_feature_check) and so on.

Oh, and looking at the patch one more time, I'm wondering if submitForm() should be buildEntity() instead. See #2834030: Currently we are saving an user entity with some not validated fields on the registration form. And we should possibly use the new time service there, not REQUEST_TIME.

Lets keep the dcmuc tag to keep "attribution" of the history :)

Haven't reviewed the whole patch yet, but I'd really like to be clear on what the expectations for the UI are. I feel like the current revision UI on entity forms is really bad, and turning that bad thing into something re-usable, just not sure where exactly it gets us to.

1. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,215 @@
   +
   +    $form['revision'] = [
   +      '#type' => 'checkbox',
   +      '#title' => $this->t('Create new revision'),
   +      '#default_value' => $new_revision_default,
   +      '#access' => !$this->entity->isNew() && $this->entity->get($entity_type->getKey('revision'))->access('update'),
   +      '#group' => 'revision_information',
   +    ];
   +
   

   While I agree we shouldn't change the node form (or the block form) in this issue, my own view is that we should aim to remove this checkbox from all revision forms (i.e. always use $new_revision_default).

   Are people working on this expecting us to change the form in a follow-up though? Maybe that's fine, but it could use a @todo to the issue (which I can't find at the moment).

2. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
   @@ -0,0 +1,215 @@
   +    // Save as a new revision if requested to do so.
   +    if (!$form_state->isValueEmpty('revision')) {
   +      $entity->setNewRevision();
   +      // If a new revision is created, save the current user as revision author.
   +      $entity->setRevisionUserId(\Drupal::currentUser()->id());
   +      $entity->setRevisionCreationTime(REQUEST_TIME);
   +    }
   

   And this is a bit tricky too. If someone creates a new revision, then someone comes along later and overwrites the revision with different content, who's the revision author really?

My feedback came from a consistency point only, agreed that overall improvements are possible. How would it work without the checkbox? Every save is a revision anyway and a message is optional?

@yoroy yes something like that. Or if revisions are off, then no UI to enable them on the entity form (you'd have to do it at bundle or entity type level).

@catch: See #48-50 for the issue I think you mean and my feedback to that. Will copy that over. Adding a @todo to that issue is fine with me, although I'm not convinced yet about forcing it.

About the revision author, not sure what else we could do. Problem goes away if you enforce revisions, so if you care about that, do that..

Setting to needs work, I think this is currently discussed and being worked on at the media sprint.

Also, I think we haven't had sufficient discussion on whether we shouldn't even allow an opt-out through the UI. I still think that is a very sane thing to do. Although there are some use-cases where you do not want that from my experience those are rather advanced so it's not unreasonable to expect a custom form for that.

To me personally this is just the logical next step of defaulting the checkbox to being checked which we now do. I obviously can't decide this on my own, but I don't feel like #48 / #50 was sufficient discussion on this.

Yes, totally agree with needing more discussion, but we have #2696555: On the entity form of revisionable entities, make "create new revision" and "revision log" configurable for this discussion. So I think adding a @todo for that issue is enough here, as suggested by #82.

I'll also write a few lines based on a discussion I just had with catch in that other issue.

For #79 we are moving the code from RevisionableContentEntityForm to ContentEntityForm. It's fair to say that having the field in a base class could lead to problems in the future.

• The code style /** @var $preview \Drupal\Core\Form\FormStateInterface */ thing is reverted.
• Instead of an annotation we use a showRevisionFormFields() function to let content entities show or hide the revision fields.
• The submitForm stuff is now moved to buildEntity as well.
• The time service is used for REQUEST_TIME. We can't use dependency injection in ContentEntityForm without breaking stuff, so we use \Drupal::time() for now.

About the UI changes, I agree it is probably better to discuss those in a seperate issue. This patch should just remove duplicate code and is probably a good base for discussing generic UI/UX improvements.

Something went wrong with the last patch...

1. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -298,6 +298,13 @@
    
      /**
   +   * {@inheritdoc}
   +   */
   +  public function showRevisionFormFields() {
   +    return TRUE;
   +  }
   +
   +  /**
   

   why not just a property, protected $showRevisionFormFields = TRUE? Also not sure about FormFields, we are doing more than just fields. showRevisionUi makes more sense to me.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -43,8 +50,41 @@ public static function create(ContainerInterface $container) {
   +
   +    // Advanced tab must be the first, because other fields rely on that.
   +    $form['advanced'] = [
   +      '#type' => 'vertical_tabs',
   +      '#weight' => 99,
   +    ];
   +
   

   this should IMHO only be added conditionally as well.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -76,6 +121,23 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    
   +    if ($entity->getEntityType()->hasKey('revision')) {
   

   same here, not sure about doing something here now that we didn't before.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -110,6 +172,15 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +  public function save(array $form, FormStateInterface $form_state) {
   +    if ($this->entity->showRevisionFormFields()) {
   +      $this->saveRevisionableFormFields($form, $form_state);
   +    }
   +  }
   

   we're saving everything, not just revisionable form fields.

   I'm not 100% sure what to do about save(). Nothing there is actually revision UI specific. So maybe it doesn't belong in this issue, we're only using it for block_content now and now actually saving any code anyway yet. Maybe a separate issue with a separate flag?

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
   @@ -53,4 +53,12 @@ public function setRevisionTranslationAffected($affected);
    
   +  /**
   +   * Checks whether the revision form fields should be added to the form.
   +   *
   +   * @return bool
   +   *   TRUE if the form field should be added, FALSE otherwise.
   +   */
   +  public function showRevisionFormFields();
   +
   

   certainly doesn't need to be on the interface/public. either a protected method on the base class or a protected property which I'd prefer, easier to define IMHO.

If we are already adding such a flag, why not make it a method, so that I can add some conditional logic in my custom entities if I want. We can even consider doing something like !$this->isNew() or something (in a follow-up, not here) in core to consolidate the pre-existing logic around that.

Oops, the status change was unintentional.

As discussed on IRC we now introduce a new annotation for EntityType's.

Also we addressed the comments from @berdir in #91

We removed EntityTestRevisionForm and introduced show_revision_ui to EntityTestRev and EntityTestMulRev

We also moved the message stuff back into the BlockContentForm, just to be sure, not to break anything.

So, pretty long review again, but mostly smaller things I think.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -76,6 +125,22 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +      else {
   +        $entity->setNewRevision(FALSE);
   +      }
   

   the explicit FALSE call is I think a left-over of when this worked by calling setNewRevision() while preparing the form. We changed that because it is not working, so until #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously is fixed, this is dead code and we could actually simply drop that.

   We could also combine the two if conditions into one then, but no strong opinion on that.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -283,8 +348,89 @@ public function updateFormLangcode($entity_type_id, EntityInterface $entity, arr
   +    if ($this->operation == 'edit') {
   

   Should we do an !$this->entity->isNew() check instaed here?

   There isn't really any requirement to even have different form operations for add/edit, term just has default and user has register/default, where default is editing.

   Also, entity_test entities also just have default, so this title logic likely doesn't work for them right now, we probably just don't have test coverage for it, because we also previously didn't have it (Adding that somewhere where entity_test_rev is edited would be a good way to ensure that it works without an edit operation)

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -283,8 +348,89 @@ public function updateFormLangcode($entity_type_id, EntityInterface $entity, arr
   +      $bundle_info = \Drupal::service('entity_type.bundle.info')->getBundleInfo($entity_type->id());
   

   I assume you removed the injection to avoid a BC change on the constructor?

   There is a half-official way to do it in a backwards-compatible way. You add the constructor argument but make it optional and fall back to \Drupal::service() in the constructor.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -283,8 +348,89 @@ public function updateFormLangcode($entity_type_id, EntityInterface $entity, arr
   +    $new_revision_default = FALSE;
   +    $bundle_entity = $this->getBundleEntity();
   +    if ($bundle_entity instanceof RevisionableEntityBundleInterface) {
   +      // Always use the default revision setting.
   +      $new_revision_default = $bundle_entity->shouldCreateNewRevision();
   +    }
   

   Should we make this default also a property or method? I might want to default to revisions on in a custom entity type but might not use bundles. Maybe we could move the whole block into a method and just do:

   $new_revision_default = $this-getNewRevisionDefault() or so.

5. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
   @@ -574,6 +574,14 @@ public function getBaseTable();
      /**
   +   * Indicates whether the revision form fields should be added to the form.
   +   *
   +   * @return bool
   +   *   TRUE if the form field should be added, FALSE otherwise.
   +   */
   +  public function showRevisionUi();
   

   Writing this here but it could be anywhere:

   I think we need a change record that describes this new annotation key and what it provides exactly. So that custom/contrib entity types know that they can start to use this.

6. +++ /dev/null
   @@ -1,8 +0,0 @@
   -drupal.block_content:
   -  version: VERSION
   -  js:
   -    js/block_content.js: {}
   -  dependencies:
   -    - core/jquery
   -    - core/drupal
   -    - core/drupal.form
   

   Is removing this an API change? I have no idea if someone somehow relied on it. If it is a problem, we could keep it and just don't use it anyore.

   I guess a JS behavior is kind of like a hook and falls under the same BC-exclusion (directly calling a hook is not an API and hook implementations can be removed in minor versions). The library might be different though, so yet another version would be to keep the library but leave it empty or something.

7. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -44,177 +19,62 @@ class BlockContentForm extends ContentEntityForm {
       */
      public function save(array $form, FormStateInterface $form_state) {
   -    $block = $this->entity;
   -
   -    // Save as a new revision if requested to do so.
   -    if (!$form_state->isValueEmpty('revision')) {
   

   Given that we don't really change it, it is probably better to just return to the exact implementation in HEAD for this method?

8. +++ b/core/modules/node/src/NodeForm.php
   @@ -11,7 +12,7 @@
     */
   -class NodeForm extends ContentEntityForm {
   +class NodeForm extends ContentEntityForm  {
    
   

   double space

9. +++ b/core/modules/node/src/NodeForm.php
   @@ -98,10 +86,6 @@ public function form(array $form, FormStateInterface $form_state) {
        // Changed must be sent to the client, for later overwrite error checking.
   

   I'm wondering if that's actually something that we could consider doing by default as well, isn't that actually required for the EntityChangedConstraint to work? Separate issue, of course

10. +++ b/core/modules/node/src/NodeForm.php
    @@ -110,47 +94,11 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $form['revision']['#access'] = $current_user->hasPermission('administer nodes') && !$node->isNew();
    

    Hm. Now that the parent uses revision field access to check visibility, we might not even need this line anymore (nice idea to do that, btw!)

11. +++ b/core/modules/node/src/NodeTranslationHandler.php
    @@ -17,17 +17,7 @@ class NodeTranslationHandler extends ContentTranslationHandler {
           // We do not need to show these values on node forms: they inherit the
           // basic node property values.
           $form['content_translation']['status']['#access'] = FALSE;
    

    also unrelated, but is this still required? I thought we have generic logic now that knows if we have a custom status or not.

    Looking... aw, nope, still hardcoded there :)

I'm wondering if that's actually something that we could consider doing by default as well, isn't that actually required for the EntityChangedConstraint to work?

Yes!!! I just recently realized, as well, that the fact that EntityChangedConstraint claims to be generically re-usable across entity types is a complete and utter lie due to this...

Here is a new patch that addresses the comments from #98

A change-record and the follow-up issues are on its way

The change record can be found here: https://www.drupal.org/node/2835025

Follow up issues planned:

• ContentEntityForm interface changed, change all forms extending it.
• Fix naming of variables in BlockContentForm save(). It uses the terms info/label in a confusing way right now.
• As mentioned in 98-9: I'm wondering if that's actually something that we could consider doing by default as well, isn't that actually required for the EntityChangedConstraint to work?

Here is a new patch.

1. showRevisionUi will now also depend on having a revision entity key
2. Added an empty js key to the library to fix the tests
3. In MenuLinkContentForm we passed a language manager to the ContentEntityForm, but it was never used. So i removed that, because tests were failing since we introduced the optional arguments for ContentEntityForm

Looking at the change record "The logic to enable revision fields on a content entity was moved to ContentEntityForm." does not make it clear where it is moving *from* ie. what's the "I have this and I should change to this".

Some notes. All minor so leaving at needs review.

This is looking really great, by the way, I am quite excited for this! Thank you @chr.fritsch for your tireless efforts on this!

Edit: X-post with @dawehner so removing some notes.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -76,6 +149,17 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +    if ($this->showRevisionUi() && !$form_state->isValueEmpty('revision')) {
   
   +++ b/core/modules/node/src/NodeForm.php
   @@ -304,32 +247,6 @@ protected function actions(array $form, FormStateInterface $form_state) {
   -    if (!$form_state->isValueEmpty('revision') && $form_state->getValue('revision') != FALSE) {
   

   Do we not need to check for a TRUE(ish) value?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -283,8 +367,99 @@ public function updateFormLangcode($entity_type_id, EntityInterface $entity, arr
   +      $bundle_info = $this->entityTypeBundleInfo->getBundleInfo($entity_type->id());
   +      if ($bundle_info[$this->entity->bundle()]) {
   +        $form['#title'] = $this->t('<em>Edit @bundle_label</em> @label', [
   +          '@bundle_label' => $bundle_info[$this->entity->bundle()]['label'],
   +          '@label' => $this->entity->label(),
   +        ]);
   +      }
   +      else {
   +        $form['#title'] = $this->t('<em>Edit</em> @label', [
   +          '@label' => $this->entity->label(),
   +        ]);
   

   Why is this included here? (I.e. both why is this in the scope of this patch and why is it in a method called addRevisionableFormFields()?)

3. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -167,19 +43,11 @@ public function form(array $form, FormStateInterface $form_state) {
   -    $block_type = $this->blockContentTypeStorage->load($block->bundle());
   +    $block_type = $this->entityTypeManager->getStorage('block_content_type')->load($block->bundle());
   

   I guess this could use $this->getBundleEntity() now?

And again:

#106
1. Fixed
2. Nothing to do
3. and 4. Fixed
5., 6., 7., 8. fixed
9. We made that deprecated
10. and 11. Fixed

#107

1. $form_state->isValueEmpty('revision') already checks that the value is not FALSE
2. Good point. We removed that and added the NodeForm logic again, because this is out of scope now
3. Fixed

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
   @@ -377,24 +377,9 @@
   +  protected function addRevisionableFormFields(&$form) {
   

   $form should be type hinted, but that could be fixed on commit. I don't want to block RTBC of this magnificent patch for that.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -800,7 +800,7 @@
   +  function testLongNameFieldIndexes() {
   

   Why did this stop being public?

+++ b/core/core.libraries.yml
@@ -203,6 +203,15 @@ drupal.dropbutton:
+drupal.entity-form:
+  version: VERSION
+  js:
+    misc/entity-form.js: {}
+  dependencies:
+    - core/jquery
+    - core/drupal
+    - core/drupal.form

I think drupal.form depends on drupal and jquery, so we can drop those two dependencies here.

Checked the last interdiffs, looks good to me as well.

Note to self: don't make recommendations without looking at the actual patch…

Create block

no has checkbox

Edit block

does has checkbox

Which is not so nice. I'm fine with #2696555: On the entity form of revisionable entities, make "create new revision" and "revision log" configurable as the place to hash out the ux for revision checkboxes or not etc.
If we can revert things so that at least the custom block experience is consistent within itself that would be great. Are there other core entities that make use of this?

That might look weird, but it is actually consistent with node forms. We recently changed them to work exactly like this.

Node and Block Content are the only entities in core that use this. It would make sense the UX is similar.

I do get the comment to remove the checkbox, but it would probably be better to work out a good default UI and implement it for all revisionable content entities in #2696555. This patch actually helps with that.

Didn't want to change the status.

See #2744877: Node add form shows 'create new revision' checkbox

Ok thanks for explaining, #2696555: On the entity form of revisionable entities, make "create new revision" and "revision log" configurable it is then.

Committed/pushed to 8.3.x, thanks! Hopefully see people over in #2696555: On the entity form of revisionable entities, make "create new revision" and "revision log" configurable.

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestRev.php
@@ -45,7 +48,9 @@
+class EntityTestRev extends EntityTest implements RevisionLogInterface {
+
+  use RevisionLogEntityTrait;

@@ -53,13 +58,8 @@ class EntityTestRev extends EntityTest {
-    $fields['revision_id'] = BaseFieldDefinition::create('integer')
-      ->setLabel(t('Revision ID'))
-      ->setDescription(t('The version id of the test entity.'))
-      ->setReadOnly(TRUE)
-      ->setSetting('unsigned', TRUE);
+    $fields += static::revisionLogBaseFieldDefinitions($entity_type);
 
-    $fields['langcode']->setRevisionable(TRUE);

This change was not needed and incorrect. EntityTestRev should not use the revision log trait, we have a dedicated test entity type for that: EntityTestWithRevisionLog.

It was introduced in #61 to "fix broken tests", but the new code for the revisionable form had to be fixed instaed, which actually happened in the final committed patch.

Opened #2835588: Restore EntityTestRev's behavior to not implement RevisionLogInterface to fix it quickly :)

Followup: #2836447: RevisionLogEntityTrait is not compatible with new show_revision_ui option.