As a prequel to #2705389: All content entities should extend RevisionableContentEntityBase Node entity should implement RevisionLogInterface.

I did start working on a patch, to make Node and BlockContent extend RevisionableContentEntityBase, but there is one big flaw. Node and BlockContent entities currently define similar fields to RevisionLogEntityTrait but use different names. If we wanted to do this there would've been three options:

  • Within Node and BlockContent update alter the field definitions set by RevisionLogEntityTrait.
    This is what I have done in the patch but it causes the getters and setters in RevisionLogEntityTrait not to work because they use a different name.
  • Introduce revision_created, revision_user, and revision_log_message entity keys, use these in RevisionLogEntityTrait instead of hardcoding field names.
    We already do this for many other fields.
  • Update RevisionLogEntityTrait to use the field names already used in Node and BlockContent.
    I don't understand why this wasn't done in the first place.

Instead I am just going to get Node and Block content to implement RevisionLogInterface. Starting with Node.

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

Issue summary: View changes
timmillwood’s picture

Issue summary: View changes
bojanz’s picture

What we care about is the interface, the base class and trait are just a way to reduce boilerplate. It's fine if we can't.

So let's make NodeInterface and friends extend the new interface. Deprecate old methods that don't match (like Node's getRevisionAuthor(), since we now have getRevisionUser())

Status: Needs review » Needs work

The last submitted patch, extend-RevisionableContentEntityBase.patch, failed testing.

timmillwood’s picture

Title: Revisionable entities should extend RevisionableContentEntityBase » Node should implement RevisionLogInterface
Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.13 KB

This patch switches Node to implement RevisionLogInterface.

It looks like Block might be trickier as it doesn't yet have a Revision user or a Revision creation time.

amateescu’s picture

The patch looks good to me, I only found one small thing:

+++ b/core/modules/node/src/Entity/Node.php
@@ -6,6 +6,7 @@
+use Drupal\Core\Entity\RevisionLogInterface;

This use statement is not needed anymore.

timmillwood’s picture

Updated based on #7.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/node/src/Entity/Node.php
@@ -325,6 +332,44 @@ public function setRevisionAuthorId($uid) {
+   * {@inheritDoc}
...
+   * {@inheritDoc}
...
+   * {@inheritDoc}
+   */
...
+   * {@inheritDoc}

Nitpick, can be fixed on commit: We use {@inheritdoc}

timmillwood’s picture

Updated based on #9.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Shouldn't the deprecated methods like getRevisionAuthor call the new methods like getRevisionUser()? I realise they're one-liners but makes the deprecation more explicit.

Where we add the deprecation notices we should say 'Use Blah\Blah instead' as well.

Also needs a follow-up to remove the usage of the deprecated functions and switch to the new ones - for example in Node::preSave().

timmillwood’s picture

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

Updated base on #11.

tormi’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/src/NodeInterface.php
@@ -156,6 +157,7 @@ public function getRevisionAuthor();
    *   The called node entity.
    *
    * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
+   *   User setRevisionUserId() instead.
    */
   public function setRevisionAuthorId($uid);
 

Minor typo: User > Use :)

timmillwood’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
522 bytes

Updated based on #13.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f8fd764 and pushed to 8.2.x. Thanks!

+++ b/core/modules/node/src/NodeInterface.php
@@ -140,6 +141,9 @@ public function setRevisionCreationTime($timestamp);
+   *
+   * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
+   *   Use getRevisionUser() instead.
    */
   public function getRevisionAuthor();

@@ -151,6 +155,9 @@ public function getRevisionAuthor();
+   *
+   * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
+   *   Use setRevisionUserId() instead.
    */
   public function setRevisionAuthorId($uid);

Can we get a follow up to replace all usages? Ideally 8.2.0 will not have any. Also the reference to the method should be fully qualified.

diff --git a/core/modules/node/src/NodeInterface.php b/core/modules/node/src/NodeInterface.php
index 46fd77c..980dacd 100644
--- a/core/modules/node/src/NodeInterface.php
+++ b/core/modules/node/src/NodeInterface.php
@@ -142,8 +142,8 @@ public function setRevisionCreationTime($timestamp);
    * @return \Drupal\user\UserInterface
    *   The user entity for the revision author.
    *
-   * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
-   *   Use getRevisionUser() instead.
+   * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0. Use
+   *   \Drupal\Core\Entity\RevisionLogInterface::getRevisionUser() instead.
    */
   public function getRevisionAuthor();
 
@@ -156,8 +156,8 @@ public function getRevisionAuthor();
    * @return \Drupal\node\NodeInterface
    *   The called node entity.
    *
-   * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.
-   *   Use setRevisionUserId() instead.
+   * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0. Use
+   *   \Drupal\Core\Entity\RevisionLogInterface::setRevisionUserId() instead.
    */
   public function setRevisionAuthorId($uid);
 

Fix made on commit.

  • alexpott committed f8fd764 on 8.2.x
    Issue #2705433 by timmillwood: Node should implement...
alexpott’s picture

Status: Fixed » Closed (fixed)

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