The interface method EntityStorageInterface::loadRevision() states that the method should return FALSE if no matching revision is found.

However, several methods that implement this do not obey this contract:
- ContentEntityDatabaseStorage::loadRevision - no return value if the query results in no revision being found
- EntityDatabaseStorage::loadRevision - throws an exception that is not documented with @throws; no return value.
- ContentEntityNullStorage::loadRevision - returns NULL

The other two methods that implement the interface do obey the "return FALSE" that is specified.

So... Either the documentation for the interface should be updated (possibly to say that the behavior if a revision does not exist or if the entity does not support revisions is up to the individual implementation, which is probably a really bad idea!), or those methods should be revised to obey the contract of the interface.

CommentFileSizeAuthor
#21 2296115-with-test-fixes-20.patch4.57 KBhussainweb
#9 interdiff.txt1.11 KBjhodgdon
#9 2296115-with-test-fixes-9.patch5.09 KBjhodgdon
#5 2296115-with-test-fixes.patch4.57 KBjhodgdon
#4 2296115-with-test-fixes.patch2.33 KBjhodgdon
#2 2296115.patch3.29 KBjhodgdon

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

return NULL is correct, all load methods were updated to return NULL, loadRevision() should follow that.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2296115.patch3.29 KB

OK, here's a patch.

What about that stray exception? It's not documented with a @throws, so I think it should not be there?

But deleteRevision() on the interface doesn't document its exceptions either, and several implementations throw them. Hm.

Status: Needs review » Needs work

The last submitted patch, 2: 2296115.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2296115-with-test-fixes.patch2.33 KB

Apparently there are a couple of tests that are expecting the other behavior. Added 2 lines to this about 10 line patch so the tests expect the new behavior.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
StatusFileSize
new 2296115-with-test-fixes.patch4.57 KB

Whoops, wrong patch.

The last submitted patch, 4: 2296115-with-test-fixes.patch, failed testing.

joachim’s picture

joachim commented 
  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -487,8 +487,14 @@ public function loadRevision($revision_id) {
+
+    // Return value must be NULL if there is no entity found.
+    return NULL;

+++ b/core/lib/Drupal/Core/Entity/EntityDatabaseStorage.php
@@ -87,7 +87,7 @@ protected function doLoadMultiple(array $ids = NULL) {
-    throw new \Exception('Database storage does not support revisions.');
+    return NULL;

    AFAIK you can just fall out of a function and that's the same as returning NULL. 

  2. +++ b/core/lib/Drupal/Core/Entity/EntityDatabaseStorage.php
@@ -87,7 +87,7 @@ protected function doLoadMultiple(array $ids = NULL) {
   public function loadRevision($revision_id) {

    Might it be desirable to throw an exception if you're requesting a revision for something that doesn't support them at all?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

You are right about the return NULL.
http://www.php.net/manual/en/functions.returning-values.php

I do not agree about the exception. The other classes that do not support revisions (like Config entities) just return NULL as the interface says they should. But maybe Berdir or someone else has an opinion? If we're going to allow an exception to be thrown, we should document it on the interface anyway.

Another note: We also need to update the docs for the bare functions in entity.inc that load revisions.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2296115-with-test-fixes-9.patch5.09 KB
new interdiff.txt1.11 KB

New patch...

dawehner’s picture

dawehner
Primary language German
commented

It seems to be that we rather want to extend the documentation to support throwing an exception.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

RE #10 - it is inconsistent then. Config entities do not support revisions, but they just return FALSE or NULL or whatever and do not throw an exception. So if we are going to document that there should be an exception if revisions are not supported, then we should do that everywhere. I thought it made more sense just to return NULL, since most of the classes that do not support revisions do that?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

if exception, then we should introduce a generic Entity\Exeption\UnsupportedMethod/Action/OperationException, because EntityDatabaseStorage also throws an exception right below in deleteRevision(), and others like the null storage throw an exception in getQueryServiceName().

joachim’s picture

joachim commented

Should the exception stuff be dealt with in a new issue, leaving this one to just deal with just fixing the inconsistent returns?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

The maintainers of Drupal 8 and/or the entity system need to decide whether an exception is a good idea or not. I personally think not. If they are a good idea, then we should deal with them on this issue; if they are not then we should also deal with them on this issue, which is about whether methods are obeying the interface contract after all (which currently does not permit exceptions).

Berdir queued 9: 2296115-with-test-fixes-9.patch for re-testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +Needs reroll, +Novice

Novice for re-roll.

The problematic storage implementation was removed I think.

Status: Needs review » Needs work

The last submitted patch, 9: 2296115-with-test-fixes-9.patch, failed testing.

amundost queued 9: 2296115-with-test-fixes-9.patch for re-testing.

The last submitted patch, 9: 2296115-with-test-fixes-9.patch, failed testing.

Palashvijay4O’s picture

Palashvijay4O commented
Assigned: Unassigned » Palashvijay4O
hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll, -Novice
StatusFileSize
new 2296115-with-test-fixes-20.patch4.57 KB

Rerolled... Removed EntityDatabaseStorage entirely as per #2332577: Remove EntityDatabaseStorage. Rest everything was handled by git itself, including renaming ContentEntityDatabaseStorage to SqlContentEntityStorage as per #2330091: Rename ContentEntityDatabaseStorage to SqlContentEntityStorage.

berdir’s picture

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

Great, thanks.

joachim’s picture

joachim commented

What about the fact that some implementations throw an exception?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

There was only one and that one was sent to /dev/null :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue summary: View changes

OK... So I went back to the issue summary and checked this over and looked at the patch:

a) The patch changes the interface docs so it says it returns NULL if there is no entity. That seems good to me, more consistent than FALSE with the rest of Drupal API and in PHP in general. Good!

b) There are now 4 classes that implement this:
- ContentEntityNullStorage - not patched here, already returned NULL
- KeyValueEntityStorage - patched here to return NULL always (was FALSE)
- SqlContentEntityStorage - patched here to not return anything (same as NULL) if there is nothing loaded. Previously returned output of reset($array), which is FALSE if $array is empty.
- ConfigEntityStorage - patched here to return NULL always (was FALSE)

c) As Berdir stated above, nothing is throwing any exceptions.

So I agree this is good. +1 for RTBC. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2296115-with-test-fixes-20.patch, failed testing.

Status: Needs work » Needs review

hussainweb queued 21: 2296115-with-test-fixes-20.patch for re-testing.

berdir’s picture

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

I assume that was a random fail.

alexpott’s picture

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

Committed 49caf9d and pushed to 8.0.x. Thanks!

  • alexpott committed 49caf9d on 8.0.x 
    Issue #2296115 by jhodgdon, hussainweb: Fixed Several entity...

Status: Fixed » Closed (fixed)

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

Palashvijay4O’s picture

Palashvijay4O commented
Assigned: Palashvijay4O » Unassigned