Support keeping new revision id for migrate

1. +++ b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
   @@ -54,6 +54,21 @@ public function getRevisionId();
      /**
   +   * Checks if this revision is editable.
   +   *
   +   * Editing revisions is very strongly discouraged.
   

   Not exactly sure how, but I think this should better explain what it is and means:

   Returns whether a new revision ID should be generated.

   When TRUE is returned, the current revision ID must be used to save the new revision. The caller is responsible to set a valid revision ID.

   ?

2. +++ b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
   @@ -54,6 +54,21 @@ public function getRevisionId();
   +   * @param bool $new_value
   +   *   (optional) A Boolean to (re)set the editable flag. DO NOT USE. This is
   +   *   solely for migrate to use.
   

   I don't think it's necessary to do repeated warnings, what I wrote above is IMHO fine. Just "The new value to set." or so should be enough.

That said, I just had an idea how to solve this without adding a new method.

What if we move the responsiblity of removing the old revision ID to setNewRevision() ? If you call setNewRevision(TRUE), then it will do $this->{$this->getEntityType()->getKey('revision_id')} = NULL.

Then all migrate needs to do is set the revision ID it wants *after* calling that? And we can remove that logic from the storage controller?

It needs the first call because it uses isNewRevision() for the revision checkbox.

I'm not sure, but I think it's needed when new revision is on by default but then disabled. That's probably broken right now because we only call it when it's said, we should call it with the value of the checkbox. Same for CustomBlockFormController.

That's an interesting point though, this means that the object would already lose the version ID while being edited and persisted like that in the form storage, could that be a problem? I'm not sure...

Yes, confirmed that the use case with default revision on but then disabling it manually does not work as expected. I'm always impressed how much stuff we're *not* testing ;)

Not a problem of this issue, but will need a new bug report.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -165,6 +165,14 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
+      $revison_key = $this->getEntityType()->getKey('revision_id');
+      if (isset($this->$revison_key)) {
+        $this->$revison_key = NULL;
+      }

I don't think the isset() is necessary, as this is a content entity where the field object should always be there (below it's a stdClass data record). You might want to do a $this->set($revision_key, NULL) instead, though.

Ah, just revision, not revision_id :) Made that mistake myself a while ago too.

Re-rolled to use revision instead of revision_id as suggested in #13. Also fixed the comment, was a touch long :)

Interesting fails:

- We didn't reset setNewRevision() after saving a revision, so when you save again, it tried to create a new revision again, only this time, it still used the old revision ID. I'm not 100% sure this is correct, but I think it's ok and consistent with isNew(), which will not save multiple new entities either (this is a bit different, though, but why would you want to create multiple new revisions by default?)
- A few calls to setNewRevision() on entity types that don't have revisions. Changed one instance to an entity type that has revisions and removed a bogus setNewRevision() in another.

This should be better.

However, while looking at that, I noticed that we forcefully call setNewRevision() when saving new entities.. so it is not possible to save a new entity with a given revision_id with the current patch :(

Works for me, did you verify that this works for migrate? I think the code that needs to be updated to use this is now actually in core, so can update it too. Too bad there are no tests for it yet :)

Then you have a different core than I do I think :)

Both EntityRevision and EntityContentBase call keepNewRevisionId(TRUE) in my core :)

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -164,7 +164,24 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
+      throw new \LogicException(String::format('Entity type @entity_type does support revisions.'));

Discussed this change in behaviour with berdir, and agreed it make sense. However, this needs to be documented, i.e. the interface documentation should tell us that this @throws this exception if the entity type is not revisionable. Opened the related #2224549: Simplify checking whether an entity type is revisionable.

Else, the patch looks good to me and cleans up things nicely. Thus setting needs work for the missing doc change.

I think @throws should be above @see and have its description below of it, updated it accordingly.

Ok, @fago approved everything else and the doc fix looks good, so RTBC!

Yep, thanks - I agree this is RTBC.

Yeah, working on it.

This is one of the days where I don't like PhpUnit ;)

Strange, now this seems to be failing the new Ui test that we added. That could be an actual problem, haven't looked into it yet.

True!

Yay test coverage! ;)

Just a re-roll.

Better.

1. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
   @@ -61,6 +62,13 @@ class ContentEntityBaseUnitTest extends UnitTestCase {
   +   * The typed data manager used for testing.
   +   *
   +   * @var \Drupal\Core\Field\FieldTypePluginManager|\PHPUnit_Framework_MockObject_MockObject
   +   */
   +  protected $fieldTypePluginManager;
   

   Field type manager.

2. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
   @@ -137,13 +137,44 @@ public function setUp() {
   +    $this->entityManager->expects($this->any())
   +      ->method('getFieldDefinition')
   

   There is no such method on the EM?

Thanks, back to RTBC then.

Looks great. Committed/pushed to 8.x, thanks!