Return NULL on EntityRepository::loadEntityByUuid() when the UUID is invalid or it doesn't exist

Nice, sounds sensible.

why FALSE and not NULL? Or should it throw an exception? Node::load(NULL) btw also gives you errors.

Oh, I see, loadEntityByUuid() is documented as return FALSE. That's inconsistent with load() in the first place :)

Also, it is consistent with load($non_existing_id), that also returns NULL.

The only problem is when you pass in NULL. That results in errors, at least with #2358079: Require a specific placeholder format in db_query() in order to trigger argument expansion, and require explicit 'IN' parameter for conditions. Which is also consistent with Node::load(NULL), that also gives you errors :p

Please review.

Should check if the UUID is empty before even trying to load it.

Please review.Thanks.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -958,6 +959,10 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
+    if(! Uuid::isValid($uuid)){

should be "if (!Uuid::" instead.

Also, a comment would be useful, something like "Immediately return NULL for empty or invalid UUID's".

Also, right now, the method is documented as return FALSE. We either need to change that to return NULL (and update the reset() to return NULL if there are no entities, as that returns FALSE) or return FALSE as well...

And there is also the option to throw an exception instead. Because calling it like this is a bug and often hints at a bigger problem in the calling code IMHO. So far, I've only seen it when doing a load(NULL), then you get the famous message about array_flip() only working for scalars. And that is usually incorrect code in the caller.

Not sure what, thoughts?

Whatever we decide, I think we should update load/loadMultiple() in a follow-up to behave the same way..

The same test fails were in #2358079: Require a specific placeholder format in db_query() in order to trigger argument expansion, and require explicit 'IN' parameter for conditions, see the changes to EntityReferenceItemNormalizer. Not sure if we should let it fail like that or check for it not being a string instead (Which would have hidden this clearly wrong code, @see exception argument :))

@Berdir,so do you think will it be a nice idea to make patch for this issue by returning NULL and commenting it.Then later as a follow up task make another issue where documentation related stuff can be deal with.
thanks

I have used both empty() and is_string() as discussed with dawehner and pwolanin on irc earlier.Also i have made some changes in the document.Not sure if i am doing this in correct manner so give your opinions.Please review.
Thanks.

Returning NULL if UUID is either empty or invalid. And Adding a comment for it as discussed above.
Please review.

Checking if the uuid is not invalid or empty, and returning null as mentioned in the /core/lib/Drupal/Core/Entity/EntityManagerInterface.php

wrong patch in previous comment. reposting.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -976,6 +981,10 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
+    if (!reset($entities)) {
+      return NULL;
+    }

if (empty($entities) is a bit easier.

Some unit tests would be nice here, looks like we actually don't have any in EntityManagerTest. If you know a bit about unit tests and mocking, then it shouldn't be too hard. Just call that method with NULL or an invalid UUID and make sure that the stuff below is never called and it returns NULL.

Here are the tests for the patch. This is my first attempt at writing tests for drupal so please give feedback on the tests.

Added the tests to my original patch from #26. Also used empty() instead of !reset().

Adding new patch and its interdiff with the previous two patches as the patch in #28 just had the tests and not the actual changes.

@prateekMehta Now I know why my patch failed testing. I had forgotten to add the actual changes. Thanks for correcting it.

@anksy, actually the way you were using getMock was also a little more complicated. I tried to use your test on the original changes it, didn't work out. check interdiff-28-30.txt , you will know.

@prateekMehta Thanks for pointing out the error. I tested the patch and it works fine.

Rerolled.

Off course.

I do not think the concerns from @Berdir in #14 have been addressed.

especially

1.

Also, right now, the method is documented as return FALSE. We either need to change that to return NULL (and update the reset() to return NULL if there are no entities, as that returns FALSE) or return FALSE as well...

2.

Whatever we decide, I think we should update load/loadMultiple() in a follow-up to behave the same way..

This follow-up should be created and made "related" to this issue.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -1006,6 +1007,10 @@ protected function getDisplayModeOptions($display_type, $entity_type_id, $includ
   +    // Return NULL for invalid UUID's.
   +    if (empty($uuid) || !Uuid::isValid($uuid)) {
   +      return NULL;
   +    }
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -1545,6 +1545,37 @@ protected function getTestHandlerClass() {
   +  public function providerTestLoadEntityByUuid() {
   

   I think the provider tests these two cases.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -1014,6 +1019,10 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
   +    if (empty($entities)) {
   +      return NULL;
   +    }
   

   but I dont see how the empty entities case is tested.

   is it?

#39 looks good to me. It catches instances in tests where loadEntityByUuid is being called with malformed UUIDs. These currently rely on those bogus UUIDs not actually existing - see #2491989: [PP-1] Use the 'uuid' database schema type (with native PostgreSQL implementation) for UUID fields.

Rerolled, along with a small typo fix.

With regards to the comments in #14 and #41:

• do you mean the EntityStorageInterface::load method? If so that does return NULL.
• added a new test to EntityUUIDTest to cover testing for NULL with a non-existent, but valid, UUID.

I did not actually realize that this was done (first patch review), so I wrote the same thing before finding the last patch--looks good!

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -1117,6 +1118,10 @@ protected function getDisplayModeOptions($display_type, $entity_type_id, $includ
+    // Return NULL for invalid UUIDs.
+    if (empty($uuid) || !Uuid::isValid($uuid)) {
+      return NULL;
+    }

This looks super defensive to me. Are we sure we want to do this? Perhaps when we have assertions in core we can use that instead.

\Drupal::entityManager()->loadEntityByUuid('node',NULL);
exception 'Drupal\Core\Database\InvalidQueryException' with message 'Query condition 'node.uuid IN ()' cannot be empty.' in /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Query/Condition.php:76

This is what happens atm - the exception produced looks fine to me. If you provide an invalid uuid it works as you expect.

@Berdir what do you think?

@berdir any feedback please? This blocks #2491989: [PP-1] Use the 'uuid' database schema type (with native PostgreSQL implementation) for UUID fields ;)

Added beta evaluation.

Patch still applies.

Seems to me that if we're throwing an EntityStorageException somewhere other than an EntityStorgeInterface class, we might be doing it wrong.

So let's move the property-exists check to EntityStorageInterface/Base where all code can benefit, and then just handle the empty-or-bad-uuid check within EntityManager.

In this PoC we rely on the query and database layers to throw exceptions validating the UUID and checking whether it's NULL. That means there isn't any validation of whether the given UUID is a valid one. It seems that should be baked in somewhere, but I'm not quite sure where.

This patch does not have tests. It passes a PHPUnit test run. Let's see what the testbot thinks.

Discuss. :-)

OK, yah, never mind. I forgot that we're not allowed to encapsulate things that should be encapsulated in one place. Our entity abstraction only lets us map a few special-case keys, and apparently can't just give us a list of properties that are allowable.

As an object lesson, note that a) All unit tests pass even though none of the functional ones do, and b) Nowhere in the process of writing that patch did the docs warn me I was making an error. I might be a fool, but Drupal is still an oral tradition.

Moving on...

Interdiff from #45. Let's all ignore #51. :-)

Removed the UUID validity checks, though there should be some way to perform that check within the query, either at loadByProperties() or some other point, since there's no point sending it off to the database to fail.

Moved loadByProperties() into a try/catch which then returns NULL for the NULL-UUID exception. The query builder's Condition class is the *only* place InvalidQueryException is thrown, specifically for empty conditions in queries. It's kind of strange, actually.

Added a bunch of unit tests.

Looks good to me. Just three observations:

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
@@ -1818,6 +1819,192 @@ protected function getTestHandlerClass() {
+  public function providerTestLoadEntityByUuid() {
...
+   * @dataProvider providerTestLoadEntityByUuid
...
+   * @dataProvider providerTestLoadEntityByUuid
...
+   * @dataProvider providerTestLoadEntityByUuid

Nitpick: Can we rename the dataprovider to providerLoadEntityByUuid.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
@@ -1818,6 +1819,192 @@ protected function getTestHandlerClass() {
+  public function testloadEntityByUuidNullQuery() {

Why no dataprovider for this test. You have one for the other tests (return ['apple', NULL];).

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
@@ -1818,6 +1819,192 @@ protected function getTestHandlerClass() {
+    $manager->loadEntityByUuid($entity_type_id, $uuid);

Can we add a comment that there is no assertion because we are expection an exception.

Thanks.

#57.1: Done.

#57.2: Like it says in the inline comments, we throw an exception in all cases, so there's no point in having a data provider. I'll expand the docs.

#57.3: The @expectedException annotation *is* the comment and also the assertion. But I'll expand on it.

Still need a follow-up, which might already exist, to do some kind of validation of UUIDs, or general validation before we hit the database.

Some related issues.

Now it looks good to me.

Thanks for fixing my issues Mile23.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -1126,8 +1128,15 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
   +    } catch (InvalidQueryException $ex) {
   

   Won't you only get this exception in the condition value is empty and it's an array? Can't we just make sure the uuid is a string before hand?

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -1126,8 +1128,15 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
   +    if (empty($entities)) {
   

   Won't reset($entities) do the same here anyway?

#61.2: reset() on an empty array returns FALSE. We want NULL.

I misread #48.

Let's try this again...

In Drupal, UUIDs are actually arbitrary strings, and some discussion is underway as to whether we should validate UUIDs: #1805576: Add a 'uuid' database schema type (Personally, I say we should add a 'uuid' column type to the Drupal schema API and add validation constraints.)

This means it's up in the air as to whether we should validate UUIDs in loadByUuid(). Let's do it, with a note that we may change policy later.

In the case that we validate UUIDs in loadByUuid(), should we throw an exception or just return NULL? Let's throw InvalidQueryException, as per #48.

We still throw EntityStorageException if the entity type doesn't support a UUID key, and we don't catch any other exceptions, so that things explode usefully.

In this patch, EntityUUIDTest does a functional check of the behavior for passing in a non-existent UUID to an entity type that supports UUIDs.

EntityManagerTest verifies the following:

• That we return an entity for the UUID when it's present.
• That we return NULL when there is no entity for the type/uuid query.
• That we throw a EntityStorageException if the entity type does not support UUID keys.
• That we throw an InvalidQueryException for malformed UUIDs.

Patch passes PHPUnit run and Entity namespace tests. Let's see what the testbot thinks.

I like the new version of loadEntityByUuid much better, thanks @Mile23!

Yah but too bad all the tests are built on non-conforming test UUIDs.

I just searched entity_test module for 'uuid' and it doesn't tell me where the UUIDs are generated or defined.

Drupal.

So it turns out the tests tell me that ConfigManager needed some love, as well as adding try/catch to loadEntityByConfigTarget().

Note that I had to break some tests, because I don't have the time to fix views_ui or editor modules. Added @todo to those. These should either be fixed here or in follow-ups.

And now fixing the modules.

Summary in #63.

Needed reroll.

Another re-roll.

Ooo. Ancient patch from before EntityManager was split into 11 classes. https://www.drupal.org/node/2549139

I guess that now it's EntityRepository::loadEntityByUuid().

Re-roll patch created, Please review.

(Wrong comment: Please ignore it.)

Actually, print_r() is used for when $uuid doesn't contain a string, but I don't see how an exception message like Invalid UUID: (output when $uuid contains FALSE or NULL) or Invalid UUID: Array() (output when $uuid contains an empty array) could be helpful. The message doesn't allow to notice the UUID is invalid because it's an empty string, FALSE, 1, or TRUE. (print_r(TRUE, TRUE) outputs 1, in the same way print_r(1, TRUE) does.)

It would be probably more helpful for the exception message to say Invalid UUID passed to EntityRepository::loadEntityByUuid().

Alternatively, the code throwing the exception could be the following one.

if (empty($uuid) || !is_string($uuid) || !Uuid::isValid($uuid)) {
  throw new InvalidQueryException('Invalid value passed for $uuid: ' . var_dump($uuid));
}

In this way, the exception message would make clear what exactly the value passed as argument is, as var_dump(TRUE), var_dump(1), var_dump(FALSE), var_dump(""), and var_dump(NULL) output different values.

Hello @kiamlaluno,
Updated the message.

Kindly review patch.

The issue summary still needs to be updated, as it speaks of returning NULL in case of bad UUID, while the patch is raising an exception. (I just changed the name of the method to alter, which changed because of splitting an entity class in multiple classes.)

See also comment #48.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -1117,6 +1118,10 @@ protected function getDisplayModeOptions($display_type, $entity_type_id, $includ
+    // Return NULL for invalid UUIDs.
+    if (empty($uuid) || !Uuid::isValid($uuid)) {
+      return NULL;
+    }

This looks super defensive to me. Are we sure we want to do this? Perhaps when we have assertions in core we can use that instead.

\Drupal::entityManager()->loadEntityByUuid('node',NULL);
exception 'Drupal\Core\Database\InvalidQueryException' with message 'Query condition 'node.uuid IN ()' cannot be empty.' in /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Query/Condition.php:76

This is what happens atm - the exception produced looks fine to me. If you provide an invalid uuid it works as you expect.

@Berdir what do you think?

The test code seems wrong.

+    $entity_type = $this->getMockBuilder('Drupal\Core\Entity\EntityTypeInterface')
+      ->setMethods(['getKey'])
+      ->getMockForAbstractClass();
+    $entity_type->expects($this->once())
+      ->method('getKey')
+      ->with('uuid')
+      ->willReturn('uuid');

getKey('uuid') should return $uuid, not a literal string.

+    $entity_storage->expects($this->once())
+      ->method('loadByProperties')
+      ->willReturn(['this entity contains uuid: ' . $uuid]);

loadByProperties() needs to return entities, not strings.

Coding standards must pass as well, https://www.drupal.org/pift-ci-job/1611745

9 coding standards messages
/var/lib/drupalci/workspace/jenkins-drupal_patches-37230/source/core/lib/Drupal/Core/Entity/EntityRepository.php
line 71 Line indented incorrectly; expected 6 spaces, found 7
/var/lib/drupalci/workspace/jenkins-drupal_patches-37230/source/core/modules/editor/editor.module
461 Expected 1 space after IF keyword; 0 found
/var/lib/drupalci/workspace/jenkins-drupal_patches-37230/source/core/tests/Drupal/Tests/Core/Entity/EntityRepositoryTest.php
14 Unused use statement
107 Short array syntax must be used to define arrays
108 Short array syntax must be used to define arrays
150 Line indented incorrectly; expected 6 spaces, found 5
150 Object operator not indented correctly; expected 6 spaces but found 5
213 @expectedException tags should not be used, use $this->setExpectedException() or $this->expectException() instead
270 @expectedException tags should not be used, use $this->setExpectedException() or $this->expectException() instead

Here i upload new patch with no drupal coding standards issues. Kindly review patch

Reroll the patch #95.

I have applied the last submitted patch #98 but it failed.
So I have tested the code and found that the culprit is DeprecatedServicePropertyTrait in EditorFileReference.php file

Needs reroll and it blocking #2491989: [PP-1] Use the 'uuid' database schema type (with native PostgreSQL implementation) for UUID fields

Re-rolling the patch for 9.1.x.

Removed dump statement.
Working on the other issues.