Entity::uriRelationships() throws exceptions if an URL cannot be generated because of missing mandatory parameters

I set the issue back to needs review, the test failure was unrelated and a retest was green.

The only thing I am not really sure about is if we should or should not return the link template in case we cannot automatically build the url for it?

Also will menu_link_content_entity_predelete be able to find the menu links without the route parameters?

I think the answer is no for both. With missing parameters, it's an invalid Url (since it did actually throw an exception while rendering), so it doesn't have a path representation, so it can't actually match any menu link. It doesn't make a practical difference, but it'd probably be better to return FALSE and filter it out.

Edit: Confirmed that if I create an entity type with a revision-revert link relationship, then create an entity and revisions, then create a menu link that points at the revert link for a specific revision, then delete the entity, the orphaned menu link will not be deleted, even after the patch. Whether we need to fix that is a separate issue, but this patch doesn't seem to fix it beyond preventing the crash. There doesn't seem to be a way to find all menu links matching a partially parameterized route (ie a specific entity ID, and any revision ID).

Update to the above: The revision relationship (the canonical link to a specific revision) automatically gets the revision parameter set by the entity, so it's not affected by this bug, and revision-specific menu links should in fact get deleted, assuming that menu_link_content_entity_predelete() is called for each revision of the deleted entity (haven't checked in detail).

So the crash only affects other link templates (such as revision_revert or revision_delete, which are autogenerated when creating the content entity type with DrupalConsole). Since it's hard to imagine a legitimate case where you would have a custom menu link pointing at a revision-deletion form, we probably don't need to go out of our way to find and delete those. At worst, you'll need to remove the orphaned menu link yourself.

I very much need this patch, but, I agree with cburschka it should really be returning FALSE. I'm just going to post a version with the boolean flipped so that I can use it in my composer file for now.

Thanks cspitzlay!

This looks ready, now we just need to remove the comment as it no longer applies.

May still need tests. (The original code was added in #2293697-188: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available, but I can't see any tests that specifically cover it specifically.)

Patch from #11 works great. I uncovered this issue is the 8.4.3 release, so it should be included in the 8.4 branch as well.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -335,6 +336,12 @@ public function uriRelationships() {
+        // In the case that a link template contains mandatory parameters we'll
+        // not be able to generate the url for it, but we still want to return
+        // the uri relationship.

"we still want to return the uri relationship" doesn't really make sense.

Plus, there already is a comment above the try. This should instead expand that.

#11 is indeed right that #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available did not add explicit tests, but that's because certain (functional) tests were failing without this change. If we want explicit test coverage for this now, then we probably want to add a testUriRelationships() to \Drupal\Tests\Core\Entity\EntityUnitTest and add mocks for the different edge cases.

Crosslinking

From my understanding the issue is actually in the way this module tries to clean up links to entity being deleted.
There is no need to loop over entity relationships when we just need a canonical URL for it.
Described approach is Implemented in patch.
Also in my case exception was caused by #2927051: Re-use parent code in RevisionableContentEntityBase::urlRouteParameters().
I think my approach could be treated not as bug fix but probably as improvement?

Thank you for that explanation @hchonov. Now that code actually make sense for me. Much appreciated.

There is an issue with this patch, which is that we can't assume that entities have a canonical URL. (e.g. paragraphs, flagging entities etc.)

@hchonov, I'd be happy to - the patch in #18 simply calls $entity->toUrl() instead of enumerating the uriRelationships method. The default parameter for that method is 'canonical' - entities that do not define a canonical link template (such as flagging entities) trigger this exception:

Drupal\Core\Entity\Exception\UndefinedLinkTemplateException: No link template 'canonical' found for the 'flagging' entity type in Drupal\Core\Entity\Entity->toUrl() (line 224 of /app/web/core/lib/Drupal/Core/Entity/Entity.php).

👌

Looking at the exceptions that url generation can throw we have:

   * @throws \Symfony\Component\Routing\Exception\RouteNotFoundException
   *   Thrown when the named route does not exist.
   * @throws \Symfony\Component\Routing\Exception\MissingMandatoryParametersException
   *   Thrown when some parameters are missing that are mandatory for the route.
   * @throws \Symfony\Component\Routing\Exception\InvalidParameterException
   *   Thrown when a parameter value for a placeholder is not correct because it
   *   does not match the requirement.

2 of which are now caught here. Thinking about InvalidParameterException I think it makes sense to ignore that one since if that was thrown something would probably be very wrong.

Giving review credit to @Wim Leers for a review that changed the patch.

Committed and pushed 7aa16e856b to 8.6.x and f51e1bde98 to 8.5.x. Thanks!