Problem/Motivation

When you want to write an entity which provides revision support you need, for example, take care about the revision form functionality on entity forms yourself.

See https://github.com/fago/entity/pull/27

Proposed resolution

Resolve that, so there is one less thing to do.

Remaining tasks

User interface changes

API changes

Data model changes

Files: 
CommentFileSizeAuthor
#112 custom-block-edit.png154.47 KByoroy
#112 custom-block-create.png155.39 KByoroy
#108 interdiff-2669802-104-108.txt8 KBchr.fritsch
#108 2669802-108.patch43.03 KBchr.fritsch
#104 interdiff-2669802-100-104.txt1.42 KBchr.fritsch
#104 2669802-104.patch43.03 KBchr.fritsch
#100 interdiff-2669802-97-100.txt9.68 KBchr.fritsch
#100 2669802-100.patch42.17 KBchr.fritsch
#97 interdiff-2669802-95-97.txt8.33 KBchr.fritsch
#97 2669802-97.patch43 KBchr.fritsch
#95 2669802-95.patch47.21 KBchr.fritsch
#95 interdiff-2669802-90-95.txt6.15 KBchr.fritsch
#90 2669802-90.patch46.76 KBchr.fritsch
#88 2669802-88.patch47.99 KBseanB
#88 interdiff-2669802-77-88.txt20.13 KBseanB
#77 interdiff-2669802-63-77.txt2.66 KBchr.fritsch
#77 2669802-77.patch45.19 KBchr.fritsch
#63 interdiff-2669802-61-63.txt4.23 KBchr.fritsch
#63 2669802-63.patch45.11 KBchr.fritsch
#61 interdiff-2669802-57-61.txt3.66 KBchr.fritsch
#61 2669802-61.patch40.28 KBchr.fritsch
#57 2669802-57.patch40.62 KBchr.fritsch
#57 interdiff-2669802-54-57.patch4.82 KBchr.fritsch
#54 interdiff-2669802-50-54.txt6.43 KBchr.fritsch
#54 2669802-54.patch37.93 KBchr.fritsch
#51 interdiff-2669802-44-50.txt10.52 KBchr.fritsch
#51 2669802-50.patch38.08 KBchr.fritsch
#45 2669802-44.patch28.93 KBchr.fritsch
#45 interdiff-2669802-41-44.txt4.98 KBchr.fritsch
#44 Bildschirmfoto 2016-12-01 um 18.36.00.png80.37 KBchr.fritsch
#44 Bildschirmfoto 2016-12-01 um 18.36.37.png45.6 KBchr.fritsch
#41 interdiff-2669802-36-41.txt4.23 KBchr.fritsch
#41 2669802-41.patch26.56 KBchr.fritsch
#40 Screen Shot 2016-12-01 at 00.16.55.png102.73 KBslashrsm
#37 interdiff-2669802-24-36.txt4.89 KBchr.fritsch
#37 add_a_content_entity-2669802-36.patch26.24 KBchr.fritsch
#37 Bildschirmfoto 2016-11-30 um 10.55.58.png116.21 KBchr.fritsch
#24 2669802-24-revisionable-form-interdiff.txt3.65 KBBerdir
#24 2669802-24-revisionable-form.patch25.5 KBBerdir
#23 2669802-23-revisionable-form-interdiff.txt11.29 KBBerdir
#23 2669802-23-revisionable-form.patch23.79 KBBerdir
#22 2669802-22-revisionable-form-interdiff.txt4.15 KBBerdir
#22 2669802-22-revisionable-form.patch14.91 KBBerdir
#19 2669802-19-revisionable-form-interdiff.txt7.71 KBBerdir
#19 2669802-19-revisionable-form.patch13.95 KBBerdir
#6 interdiff.txt3.15 KBbojanz
#6 2669802-6-revisionable-form.patch9.06 KBbojanz
#3 2669802-3.patch9.04 KBdawehner

Comments

dawehner created an issue. See original summary.

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Status: Active » Needs review
FileSize
9.04 KB

Here is a quick start of it.

bojanz’s picture

Status: Needs review » Needs work
+use Drupal\entity\Entity\RevisionableEntityBundleInterface;

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

The last submitted patch, 3: 2669802-3.patch, failed testing.

bojanz’s picture

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.

Berdir’s picture

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.

bojanz’s picture

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.

Berdir’s picture

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* :)

dawehner’s picture

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

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 ...

andypost’s picture

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

why not reuse for node form?

andypost’s picture

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

dawehner’s picture

why not reuse for node form?

Well, my goal was to get something done and usable for other code. We can still do conversions later (or actually decide to do the node conversion in this issue).

Wim Leers’s picture

  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.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

slashrsm’s picture

This is a blocker for media_entity in core.

Gábor Hojtsy’s picture

Issue tags: +blocker
Berdir’s picture

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

Berdir’s picture

Status: Needs review » Needs work
  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.

The last submitted patch, 19: 2669802-19-revisionable-form.patch, failed testing.

Berdir’s picture

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.

Berdir’s picture

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.

Berdir’s picture

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

The last submitted patch, 22: 2669802-22-revisionable-form.patch, failed testing.

The last submitted patch, 23: 2669802-23-revisionable-form.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 24: 2669802-24-revisionable-form.patch, failed testing.

The last submitted patch, 24: 2669802-24-revisionable-form.patch, failed testing.

The last submitted patch, 22: 2669802-22-revisionable-form.patch, failed testing.

The last submitted patch, 24: 2669802-24-revisionable-form.patch, failed testing.

andypost’s picture

  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

Berdir’s picture

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.

slashrsm’s picture

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?

:)

chr.fritsch’s picture

Issue tags: +dcmuc16
chr.fritsch’s picture

Assigned: Unassigned » chr.fritsch
Berdir’s picture

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.

chr.fritsch’s picture

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.

revision log

Fixed that, too.

slashrsm’s picture

Status: Needs work » Needs review

The last submitted patch, 6: 2669802-6-revisionable-form.patch, failed testing.

slashrsm’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
102.73 KB
  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).

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
26.56 KB
4.23 KB

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.

Status: Needs review » Needs work

The last submitted patch, 41: 2669802-41.patch, failed testing.

Berdir’s picture

  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.

chr.fritsch’s picture

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
revision form

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

I will work on 2, 4 and 5 tomorrow.

chr.fritsch’s picture

Forgot the patch..

slashrsm’s picture

Status: Needs work » Needs review
chr.fritsch’s picture

Assigned: Unassigned » chr.fritsch

I am on it, new patch will follow

tstoeckler’s picture

@tim.millwood just brought up #2696555: Remove the option to create a new revision or not from the node form 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.

timmillwood’s picture

Issue tags: +Workflow Initiative

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

Berdir’s picture

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?

chr.fritsch’s picture

Assigned: chr.fritsch » Unassigned
FileSize
38.08 KB
10.52 KB

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

dawehner’s picture

This looks really great already!

  1. +++ b/core/core.libraries.yml
    @@ -203,6 +203,15 @@ drupal.dropbutton:
    +drupal.entity:
    ...
    +    misc/entity.js: {}
    

    I'm wondering whether we can find a better name here which makes it clear that this is about the edit form, maybe drupal.entity_form or so.

  2. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
    @@ -0,0 +1,199 @@
    +   * @var \Drupal\Core\Entity\EntityInterface|\Drupal\Core\Entity\RevisionableInterface|\Drupal\Core\Entity\EntityRevisionLogInterface
    

    Is there a reason we don't use \Drupal\Core\Entity\ContentEntityInterface|\Drupal\Core\Entity\EntityRevisionLogInterface? The form is targeted for content entities, as it extends ContentEntityForm

  3. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
    @@ -0,0 +1,199 @@
    +    $form['advanced'] = [
    +      '#type' => 'vertical_tabs',
    +      '#weight' => 99,
    +    ];
    

    Note: the node module sets '#attributes' => array('class' => array('entity-meta')), here. I'm wondering whether we should include that as well.

  4. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
    @@ -0,0 +1,199 @@
    +    $entity_type = $this->entity->getEntityType();
    ...
    +      $bundleInfo = $this->entityTypeBundleInfo->getBundleInfo($entity_type->id());
    

    Let's ensure to use snake case for now, otherwise people will stumble upon it.

  5. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
    @@ -0,0 +1,199 @@
    +    // Add a log field if the "Create new revision" option is checked, or if the
    +    // current user has the ability to check that option.
    ...
    +      '#access' => $new_revision_default || ($entity_type->get('admin_permission')) && $account->hasPermission($entity_type->get('admin_permission')),
    

    I'm wondering whether the right approach would rather be to use field access of the revision_information field, so $entity->access('update')

  6. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionableContentEntityForm.php
    @@ -0,0 +1,199 @@
    +      '#group' => 'advanced',
    ...
    +      '#group' => 'revision_information',
    

    I'm confused that these are different groups

  7. +++ 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],
    +        ],
    +      ];
    +    }
    

    I really like these details. Well done!

  8. +++ b/core/modules/system/tests/modules/entity_test/src/EntityTestRevisionForm.php
    @@ -0,0 +1,40 @@
    +/**
    + * @file
    + * Contains \Drupal\entity_test\EntityTestRevisionForm.
    + */
    +
    

    Let's clean this up quickly and remove it.

slashrsm’s picture

Status: Needs review » Needs work
chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
37.93 KB
6.43 KB

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

slashrsm’s picture

Status: Needs review » Needs work

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).

slashrsm’s picture

+++ 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.

chr.fritsch’s picture

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.

The last submitted patch, 57: interdiff-2669802-54-57.patch, failed testing.

slashrsm’s picture

Status: Needs review » Needs work
  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

The last submitted patch, 57: 2669802-57.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
40.28 KB
3.66 KB

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

Status: Needs review » Needs work

The last submitted patch, 61: 2669802-61.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
45.11 KB
4.23 KB

Fixing the tests...

yoroy’s picture

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

Berdir’s picture

@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)

Berdir’s picture

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.

tstoeckler’s picture

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.

Berdir’s picture

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 ..

Gábor Hojtsy’s picture

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 ;)

tstoeckler’s picture

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...

yoroy’s picture

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.

Berdir’s picture

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?

tstoeckler’s picture

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?

Berdir’s picture

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

Berdir’s picture

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.

tstoeckler’s picture

Ahh, Ok. That makes Sense, Thank you!

chr.fritsch’s picture

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

seanB’s picture

Status: Needs review » Reviewed & tested by the community

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!

Berdir’s picture

+++ 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.

chr.fritsch’s picture

Gábor Hojtsy’s picture

Issue tags: +dcmuc16

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

catch’s picture

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?

yoroy’s picture

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?

catch’s picture

@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).

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

@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.

tstoeckler’s picture

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.

Berdir’s picture

Yes, totally agree with needing more discussion, but we have #2696555: Remove the option to create a new revision or not from the node form 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.

seanB’s picture

Status: Needs work » Needs review
FileSize
20.13 KB
47.99 KB

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.

Status: Needs review » Needs work

The last submitted patch, 88: 2669802-88.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
46.76 KB

Something went wrong with the last patch...

Berdir’s picture

Status: Needs review » Needs work
  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.

tstoeckler’s picture

Status: Needs work » Needs review

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.

tstoeckler’s picture

Status: Needs review » Needs work

Oops, the status change was unintentional.

The last submitted patch, 90: 2669802-90.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
6.15 KB
47.21 KB

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

Also we addressed the comments from @berdir in #91

Status: Needs review » Needs work

The last submitted patch, 95: 2669802-95.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
43 KB
8.33 KB

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.

Berdir’s picture

Status: Needs review » Needs work

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: [PP-1] setNewRevision is faulty 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 :)

tstoeckler’s picture

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...

chr.fritsch’s picture

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

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

chr.fritsch’s picture

Status: Needs work » Needs review
seanB’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 100: 2669802-100.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
43.03 KB
1.42 KB

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
Gábor Hojtsy’s picture

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".

dawehner’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -22,13 +23,41 @@ class ContentEntityForm extends EntityForm implements ContentEntityFormInterface
    +    $this->entityTypeBundleInfo = $entity_type_bundle_info ? $entity_type_bundle_info : \Drupal::service('entity_type.bundle.info');
    +    $this->time = $time ? $time : \Drupal::service('datetime.time');
    

    Note: you could use ?: here

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -76,6 +149,17 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    +    // Save as a new revision if requested to do so.
    +    if ($this->showRevisionUi() && !$form_state->isValueEmpty('revision')) {
    +      $entity->setNewRevision();
    +      if ($entity instanceof RevisionLogInterface) {
    +        // If a new revision is created, save the current user as
    +        // revision author.
    +        $entity->setRevisionUserId(\Drupal::currentUser()->id());
    +        $entity->setRevisionCreationTime(\Drupal::time()->getRequestTime());
    +      }
    +    }
    

    It is sad that we need to set the time explicitly instead of having this logic on the field item level, just with the changed field, but that's for sure not the fault of this particular patch.

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -76,6 +149,17 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    +        $entity->setRevisionUserId(\Drupal::currentUser()->id());
    

    This could use $this->currentUser()

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -76,6 +149,17 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    +        $entity->setRevisionCreationTime(\Drupal::time()->getRequestTime());
    

    This could use $this->time->getRequestTime() instead

  5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -283,8 +367,99 @@ public function updateFormLangcode($entity_type_id, EntityInterface $entity, arr
    +  public function addRevisionableFormFields(&$form) {
    

    Is there a reason this is a public method? I like to have this separated into its own method

  6. +++ 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()]) {
    

    This if() is a bit weird. I would expected that there is a notice happening as $bundle_info will be an empty array in this particular case.

  7. +++ b/core/modules/block_content/block_content.libraries.yml
    @@ -1,8 +1,6 @@
    +# This is just for BC and not used anymore. See 2669802
    

    Nitpick: let's put a URL

  8. +++ b/core/modules/block_content/src/BlockContentForm.php
    @@ -44,121 +19,22 @@ class BlockContentForm extends ContentEntityForm {
         if ($this->operation == 'edit') {
    -      $form['#title'] = $this->t('Edit custom block %label', array('%label' => $block->label()));
    +      $form['#title'] = $this->t('Edit custom block %label', ['%label' => $block->label()]);
         }
    

    Nitpick: unnecessary change

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

    We should mark isNewRevision as deprecated

  10. +++ b/core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php
    @@ -123,6 +123,11 @@ public function testNormalize() {
           'non_rev_field' => array(),
    +      'revision_created' => array(
    +        array('value' => $this->entity->getRevisionCreationTime()),
    +      ),
    +      'revision_user' => array(),
    +      'revision_log_message' => array(),
    

    For testing purposes it would be nice to setup the entity with a revision user and log message, so we can ensure it actually works as expected

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

    Nitpick: Let's not change that as it might break the patch which tries to fix them all

tstoeckler’s picture

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?

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
43.03 KB
8 KB

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

phenaproxima’s picture

  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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This patch now looks totally reasonable good for me

Berdir’s picture

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

yoroy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
155.39 KB
154.47 KB

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: Remove the option to create a new revision or not from the node form 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?

Berdir’s picture

Status: Needs review » Needs work

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

seanB’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
yoroy’s picture

Status: Needs review » Reviewed & tested by the community

  • catch committed f4a242f on 8.3.x
    Issue #2669802 by chr.fritsch, Berdir, seanB, bojanz, dawehner, yoroy,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x, thanks! Hopefully see people over in #2696555: Remove the option to create a new revision or not from the node form.

amateescu’s picture

+++ 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 :)

Chi’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.