Committed 2e45e93 and pushed to 8.8.x. Thanks!

CommentFileSizeAuthor
#5 interdiff.3069043.4-5.txt649 bytesmikelutz
#5 3069043-5.drupal.Properly-deprecate-ContentEntityStorageBasedoLoadRevisionFieldItems.patch3.34 KBmikelutz
#4 3069043-4.drupal.Properly-deprecate-ContentEntityStorageBasedoLoadRevisionFieldItems.patch3.33 KBmikelutz

Comments

mikelutz created an issue. See original summary.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

So, this is fun.

  /**
   * Actually loads revision field item values from the storage.
   *
   * @param int|string $revision_id
   *   The revision identifier.
   *
   * @return \Drupal\Core\Entity\EntityInterface|null
   *   The specified entity revision or NULL if not found.
   *
   * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0.
   *   \Drupal\Core\Entity\ContentEntityStorageBase::doLoadMultipleRevisionsFieldItems()
   *   should be implemented instead.
   *
   * @see https://www.drupal.org/node/2924915
   */
  abstract protected function doLoadRevisionFieldItems($revision_id);

  /**
   * Actually loads revision field item values from the storage.
   *
   * @param array $revision_ids
   *   An array of revision identifiers.
   *
   * @return \Drupal\Core\Entity\EntityInterface[]
   *   The specified entity revisions or an empty array if none are found.
   */
  protected function doLoadMultipleRevisionsFieldItems($revision_ids) {
    $revisions = [];
    foreach ($revision_ids as $revision_id) {
      $revisions[] = $this->doLoadRevisionFieldItems($revision_id);
    }

    return $revisions;
  }

I think ContentEntityBase::doLoadMultipleRevisionFieldItems() needs to be abstract, and the extending storage classes need to add their loading logic directly. So we should trigger an error from here, and make it deprecated for inheriting classes to call parent::doLoadMultipleRevisionsFieldItems(), (core doesn't) and then make the method abstract in D9

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> o we should trigger an error from here, and make it deprecated for inheriting classes to call parent::doLoadMultipleRevisionsFieldItems(), (core doesn't) and then make the method abstract in D9

Yes, that was the idea, IIRC. This was done by @hchonov IIRC.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Title: Properly deprecate ContentEntityStorageBase::doLoadRevisionFieldItems() » Trigger an error on direct access of ContentEntityStorageBase::doLoadMultipleRevisionFieldItems() and mark it to be set abstract in Drupal 9
Status: Active » Needs review
StatusFileSize
new 3069043-4.drupal.Properly-deprecate-ContentEntityStorageBasedoLoadRevisionFieldItems.patch3.33 KB
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
StatusFileSize
new 3069043-5.drupal.Properly-deprecate-ContentEntityStorageBasedoLoadRevisionFieldItems.patch3.34 KB
new interdiff.3069043.4-5.txt649 bytes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks good. Not sure if we need the @todo with the link and the @see.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Did this on commit, @see implies further reading rather than further work to me.

diff --git a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
index 4f52ac63e1..95a1bab294 100644
--- a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -620,8 +620,6 @@ public function loadMultipleRevisions(array $revision_ids) {
    *
    * @todo Remove this logic and make the method abstract in
    *   https://www.drupal.org/project/drupal/issues/3069696
-   *
-   * @see https://www.drupal.org/node/3069692
    */
   protected function doLoadMultipleRevisionsFieldItems($revision_ids) {
     @trigger_error('Calling ' . __NAMESPACE__ . 'ContentEntityStorageBase::doLoadMultipleRevisionsFieldItems() directly is deprecated in drupal:8.8.0 and the method will be made abstract in drupal:9.0.0. Storage implementations should override and implement their own loading logic. See https://www.drupal.org/node/3069692', E_USER_DEPRECATED);

  • catch committed 2e45e93 on 8.8.x 
    Issue #3069043 by mikelutz: Trigger an error on direct access of...
catch’s picture

catch
he/him
Primary language English
commented
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

Oh, those were two different links.. the @see was the CR explaining that calling the method directly was deprecated, the @todo linked to the followup issue.

The CR is still linked in the @trigger_error though, so it should be fine.

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented

publish change record