Remove entity_revision* family of functions usage from the code base

In the test Drupal\field\Tests\FormTest on line 578 there is the following comment: "$entity = entity_revision_load($entity_type, $entity->getRevisionId());". Can we replace that usage also?

All other usages for entity_revision_load() and entity_revision_delete() have been removed from the code base. The patch looks great. Almost RTBC for me.

Applied patch as per said in #9.

Thanks!

Merging the patches in #7 and #12. Interdiff is based on #7.

Looks good to me.
All usages for entity_revision_load() and entity_revision_delete() have been removed from the code base.
The patch looks great.

I have one nitpick left, but that can be done on commit.

+++ b/core/modules/field/src/Tests/FormTest.php
@@ -575,7 +575,9 @@ function testFieldFormAccess() {
+    $entity = $this->container->get('entity_type.manager')
+        ->getStorage($entity_type)
+        ->loadRevision($entity->getRevisionId());

Sorry for nitpicking, but if you do this multiline then the second and third lines should be only 2 spaces indented and not 4 spaces. As an alternative you can put it on a single line.

diff --git a/core/modules/field/src/Tests/FormTest.php b/core/modules/field/src/Tests/FormTest.php
index d7d9f2c..f868f7a 100644
--- a/core/modules/field/src/Tests/FormTest.php
+++ b/core/modules/field/src/Tests/FormTest.php
@@ -576,8 +576,8 @@ function testFieldFormAccess() {
 
     // Check that the revision is also saved in the revisions table.
     $entity = $this->container->get('entity_type.manager')
-        ->getStorage($entity_type)
-        ->loadRevision($entity->getRevisionId());
+      ->getStorage($entity_type)
+      ->loadRevision($entity->getRevisionId());
     $this->assertEqual($entity->$field_name_no_access->value, 99, 'New revision has the expected value for the field with no edit access.');
     $this->assertEqual($entity->$field_name->value, 2, 'New revision has the expected value for the field with edit access.');
   }

Fixed on commit.

Committed 4d8efc4 and pushed to 8.3.x. Thanks!

diff --git a/core/modules/node/node.admin.inc b/core/modules/node/node.admin.inc
index f7bc397..dc1be65 100644
--- a/core/modules/node/node.admin.inc
+++ b/core/modules/node/node.admin.inc
@@ -6,7 +6,6 @@
  */
 
 use Drupal\node\NodeInterface;
-use Drupal\node\Entity\Node;
 
 /**
  * Updates all nodes in the passed-in array with the passed-in field values.

Removed unused use on commit.

--- a/core/modules/field/src/Tests/FormTest.php
+++ b/core/modules/field/src/Tests/FormTest.php
@@ -575,7 +575,9 @@ function testFieldFormAccess() {
     $this->assertEqual($entity->$field_name->value, 2, 'New revision has the expected value for the field with edit access.');
 
     // Check that the revision is also saved in the revisions table.
-//    $entity = entity_revision_load($entity_type, $entity->getRevisionId());
+    $entity = $this->container->get('entity_type.manager')
+        ->getStorage($entity_type)
+        ->loadRevision($entity->getRevisionId());

This is uncommenting code apart from replacing the deprecated function (and fixing indentation). Nothing seems to have broken, but quite a big change for this issue.

Seriously, this either needs a revert or some feedback. Why did we bring back that dead code in #12 and #13? Was it on purpose or a by accident? We could have converted dead code without uncommenting, we could have deleted dead code, but bringing it back in a conversion issue without any comment whatsoever is.. weird.

@Eric_A it is a test - let's worry about the important things. The commenting out erroneously occurred in #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest (probably as part of getting the patch to work but was missed on review). The test is pretty meaningless - but it is hardly alone in that.

Having this in 8.3.x but not in 8.2.x does make it harder to port some 8.3.x patches or forward port 8.2.x patches, like for example coding standard patches.

So we had erroneous changes to FormTest twice, but luckily the test is pretty meaningless. Sigh, ok. Thanks for the feedback.

@Eric_A well in that case just target 8.3.x first and if there's energy to backport to 8.2.x so be it.

well in that case just target 8.3.x first and if there's energy to backport to 8.2.x so be it.

I do understand there's some hesitance to cherry-pick this task to 8.2.x now that it is in beta, but is the above really the policy for 8.2.x bug fixes? That would be a shame, cause then 8.2.x became half dead right after 8.3.x opened. Some policies are hard to track, I thought the policy here was to open bug fixes against the current minor (Not sure if that would be 8.1.x or 8.2.x right now) even if we commit to the latest open branch first.