EntityRepository::loadEntityByUuid() implementation does not match EntityRepositoryInterface::loadEntityByUuid() @return documentation

Thanks for the patch and review! However:

+++ b/core/lib/Drupal/Core/Entity/EntityRepositoryInterface.php
@@ -22,8 +22,8 @@
+   * @return \Drupal\Core\Entity\EntityInterface|bool

|bool should be |false if the only possibility is FALSE (not TRUE as well).

See
https://www.drupal.org/node/1354#types

Thanks for review. Have updated the patch.

Thanks!

Thanks again! Committed to both 8.x branches.

Instead of fixing an 8.0 implementation, we changed the 8.0 API. Changing an interface return type isn't a documentation change, it's an API change.
Are we sure about this? Perhaps a revert is in order.

I don't know what other similar interfaces look like, regardless I think this change needed more context and an explicit decision on wether a bug fix or documentation/ API change was in order.

Seems this was only ever pushed to the 8.0.x branch, not the 8.1.x one?

It's in both. The process that puts commit messages in issues is not working right, but if you look at the commit log in the actual git repo, it's there.

http://cgit.drupalcode.org/drupal/commit/?h=8.1.x&id=a172ab5b722fd85d313...

And regarding the API, the interface had been incorrectly documented.

And regarding the API, the interface had been incorrectly documented.

With all due respect, I have no idea what this means.

Personally I don't have strong feelings about this particular 8.0 API break, but I feel strongly that changing the documented return type of a public interface method is an API change. It doesn't matter how many core implementations did not respect the documented public API. In this case it seems to be just one.

API docs are not just documentation. (EDIT: And they're not about documenting existing important implementations either.) They *are* the public API that implementations should follow.

OK, let's step back here.

Yes, ideally, the API for an interface is defined by the documentation.

However, the Drupal project is not always the "ideal world". The developer who writes the interface docs may be careless, or their plan for the interface may change between when they make the interface docs and actually implement the classes that implement and use the interface.

So let's take a look at this interface and how it is documented, defined, and used (before this patch). We're talking about

EntityRepositoryInterface::loadEntityByUuid()

There are 3 classes in Core that implement this interface, see:
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!EntityRepo...

Correspondingly, there are 3 methods that implement this. But two of them essentially defer to EntityRepository::loadEntityByUuid(), which, as discussed in this issue, returns reset() on an array, which, as discussed here, returns FALSE if the array is empty.

Incidentally, loadEntityByConfigTarget() has the same problem, since it calls loadEntityByUuid() and passes the return value on.

So... that seems to indicate that whoever wrote the documentation was sloppy.

Now let's look at who calls this method:
- Lots of tests.

- Various places that look something like:

if ($entity = $someClass->loadEntityByUuid()) {
   do something;
}

so they will not care if it is FALSE or NULL.

- Some places that are not even checking for a FALSE/NULL value because they're assuming the load works.

I mean... really, for the most part it's "Does this thing load an $entity object?" and most users of the code will just test for it evaluating to Boolean TRUE. I think we did the right thing here. We should fix the other method though... or we can revert this and fix the code, but I think that would most likely break some tests.

Yes, this most probably is just a very small BC-compatible change, not a break.(EDIT: Except, some people might have done an explicit strict check on NULL...) It's a change nevertheless and thus a change notice is in order, assuming this is an interface that contrib might have implemented to extend/ swap out core implementations.

The most important thing is not discussed here, though. Why exactly do we consider changing NULL to FALSE an improvement for 8.x?

For 8.0 I still think this should be reverted and we should fix offending implementations instead.

I get the impression that we just bended the API to match an implementation instead of asking ourselves if this change is an improvement. In fact, I think there is an issue about changing loaders to return NULL or an iterable.

We are not getting anywhere with this discussion. I think we need one of the other maintainers of Drupal Core to decide this.

So, assigning to @alexpott.

Alex: To summarize:
- This issue was filed saying the return value of the EntityRepository::loadEntityByUuid() class is not matching its documentation, which is on the EntityRepositoryInterface (FALSE vs. NULL in the case of not finding the entity).
- The patch, which I RTBCd and left for 6 days, and then committed, changed the documentation of the interface to match the actual value, on the theory that the interface docs were just sloppy and it was a documentation error.
- Eric_A is complaining that this should be reverted as it is technically an API change to change interface docs.

Can you please decide? We either need to:

a) Revert the committed patch, and change the code for the class so it matches its interface docs.

b) Leave things as they are and treat it as a documentation error by the original authors of the docs, along with a trivial "API change" that we don't need to announce. If so we should also fix the docs of the loadEntityByConfigTarget() method on the same class/interface, because it has the same problem.

c) Leave things as they are but create a change notice for this change.

And the decision may not be the same for 8.0.x and 8.1.x... not sure what to do.

Changing the return of the actual implementing class (likely the only one in existence) is more of an API change than changing the return docs, so I'd go for changing the docs in this case.

Changing the return of the actual implementing class (likely the only one in existence) is more of an API change than changing the return docs

Changed API docs means that existing implementations and consumers in the wild will have to change their implementations. That code might be based on the interface API docs, not persé on an offending core implementation. There's probably few or none in the wild, but we don't know.

I was under the impression that after 8.0.0 Drupal wants to take semver seriously, and take API docs seriously. Just trying to help in that respect.

Changed API docs means that existing implementations and consumers in the wild will have to change their implementations. That code might be based on the interface API docs, not persé on an offending core implementation.

There are two possible types of code that could be affected:

1. Callers of EntityRepository::loadEntityByUuid() - if we change what the class returns.

2. Implementers of EntityRepositoryInterface::loadEntityByUuid() - by changing the docs on the interface.

In the case of the latter, if they don't do anything, they're no more or less wrong than EntityRepository currently is now.

In the case of the former, if they check for NULL explicitly, they'll be broken with the core implementation, if they check for FALSE explicitly, they'll be broken if the core implementation changes.

Once PHP introduces type hinting for return values, then this becomes more of an issue. Right now I don't see how changing the return value of the class people are actually using in 99.9% (if not 100%) of cases is less of an API change than changing what is only documentation until PHP allows type hinting of return values.

In the case of the former, if they check for NULL explicitly, they'll be broken with the core implementation, if they check for FALSE explicitly, they'll be broken if the core implementation changes.

My understanding of semver is that you resolve this kind of ugliness by fixing the buggy implementations in patch releases, and changing interfaces in minor/major releases. One might even decide to rush the minor release to stop the bleeding fast. I could certainly be wrong, though.

And also, as @catch pointed out, this is not even a backwards compatible API change. We're going to change the API like that in a patch release without even a single try to just fix an implementation. It's not like there's something really bad with a loader returning NULL...

Note that this is about changing a public interface, not about a particular implementations doc block. You can't have more of a public API than a public interface.

I've reverted this for now so it's not being discussed on a deadline - I'd not realised this was already committed when I first commented.

I don't think we should change the return value in a patch release either, so leaving at 8.1.x.

This is no longer a docs issue, or at least it is no longer *only* a docs issue.

I applied the following patch-
2625138-loadEntityByUuid_return_doc_fix-30.patch

It worked for me.

This had a framework manager review in #22

Returning NULL is indeed consistent with the single entity load methods.

It makes sense to unify with \Drupal\Core\Entity\EntityStorageInterface::load()

Yes agreed with changing the return value. In either case the real-terms breakage is theoretical so we should do what's clearest.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!