node_revision delete was not returning any value. The documentation says that it should return bool

/**
 * Deletes a node revision.
 *
 * @param $revision_id
 *   The revision ID to delete.
 *
 * @return bool
 *   TRUE if the revision deletion was successful; otherwise, FALSE.
 */
function node_revision_delete($revision_id) {
  entity_revision_delete('node', $revision_id);
}
CommentFileSizeAuthor
#11 interdiff-2737743-6-11.txt421 bytesanoopjohn
#11 drupal-node_revision_delete-does-not-return-bool-2737743-11.patch650 bytesanoopjohn
#9 interdiff-2737743-2-6.txt577 bytesanoopjohn
#6 interdiff-2737743-2-6.patch577 bytesanoopjohn
#6 drupal-node_revision_delete-does-not-return-bool-2737743-6.patch646 bytesanoopjohn
#2 drupal-node_revision_delete-does-not-return-bool-2737743-2.patch477 bytesanoopjohn

Comments

anoopjohn created an issue. See original summary.

anoopjohn’s picture

anoopjohn commented
StatusFileSize
new drupal-node_revision_delete-does-not-return-bool-2737743-2.patch477 bytes

Here is a patch that returns the return from entity_revision_delete . However I see that entity_revision_delete also does not return anything. Not sure what is the right fix for this.

entity_revision_delete calls 

\Drupal::entityManager()->getStorage($entity_type)->deleteRevision($revision_id);
anoopjohn’s picture

anoopjohn commented
Status: Active » Needs review
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

node_revision_delete() just wraps entity_revision_delete().

entity_revision_delete() isn't documented as returning anything, and doesn't.

Also entity_revision_delete() is deprecated, so node_revision_delete() should call \Drupal::entityManager()->getStorage('node')->deleteRevision($revision_id); and return whatever it returns.

Since $storage->deleteRevision() returns void, let's remove the @returns from the docblock.

I just filed this issue, as well: #2741249: Remove entity_revision* family of functions usage from the code base

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Related issues: +#2741249: Remove entity_revision* family of functions usage from the code base
anoopjohn’s picture

anoopjohn commented
StatusFileSize
new drupal-node_revision_delete-does-not-return-bool-2737743-6.patch646 bytes
new interdiff-2737743-2-6.patch577 bytes

Thanks for the review Mile23. I have made changes as suggested. Please find attached the patch and the interdiff.

anoopjohn’s picture

anoopjohn commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: interdiff-2737743-2-6.patch, failed testing.

anoopjohn’s picture

anoopjohn commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2737743-2-6.txt577 bytes

Uploaded the interdiff file with the correct extension.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work 
+++ b/core/modules/node/node.module
@@ -463,14 +463,11 @@ function node_revision_load($vid = NULL) {
+  \Drupal::entityManager()->getStorage('node')->deleteRevision($revision_id);

Sorry, the entity manager service is also deprecated. We want the entity type manager here.

anoopjohn’s picture

anoopjohn commented
Status: Needs work » Needs review
StatusFileSize
new drupal-node_revision_delete-does-not-return-bool-2737743-11.patch650 bytes
new interdiff-2737743-6-11.txt421 bytes

Thanks again for the review Mile23. Rerolled with the suggested changes

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Reviewed & tested by the community

Nice, thanks.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Blocking #2722647: [Meta] Fix Drupal.Commenting.FunctionComment.InvalidNoReturn

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Nice find and the fix looks correct - the @return is not right.

Committed 998bdf2 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 0d1fb1b on 8.2.x 
    Issue #2737743 by anoopjohn, Mile23: node_revision_delete() not...

  • alexpott committed 998bdf2 on 8.1.x 
    Issue #2737743 by anoopjohn, Mile23: node_revision_delete() not...

Status: Fixed » Closed (fixed)

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