Route definitions using entity type paramconverters with serial IDs should add an integer requirement

+++ b/core/modules/block_content/block_content.routing.yml
+++ b/core/modules/block_content/block_content.routing.yml
@@ -44,6 +44,7 @@ entity.block_content.canonical:

@@ -44,6 +44,7 @@ entity.block_content.canonical:
     _admin_route: TRUE
   requirements:
     _entity_access: 'block_content.update'
+    block_content: \d+
 
 entity.block_content.edit_form:
   path: '/block/{block_content}'

Let's also add it onto edit/delete/view and maybe even more things like all the revision UI ones.

+++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
@@ -128,8 +170,33 @@ protected function getDeleteFormRoute(EntityTypeInterface $entity_type) {
+      return NULL;

It is still odd to return NULL in that, but I just guess there is no such thing as 'any' or 'mixed' in the typed data world.

Also realized that getFieldStorageDefinitions only supports fieldable entity types.

Does that mean we ideally should add some test coverage for a test config entity type?

This seems like a good tightening of the route definitions to me. +1.

I defer to catch if it's RC-OK, but it's all straightforward enough and testbot is happy with it. If Tim says it causes problems for Page Manager without this, I will take his word for it. :-)

This seems like a pretty significant BC break. If I have an entity type with serial entity IDs, what happens to it after this patch?

Thanks @tim.plunkett! Based on that, @effulgentsia and I agreed on this being an RC target.

This patch looks great. Just a couple questions:

+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidator.php
@@ -50,9 +52,17 @@ public function validate($value, Constraint $constraint) {
+        catch (MissingMandatoryParametersException $e) {
+          $allowed = FALSE;
+        }

What is triggering this now that wasn't in HEAD? Or in other words, are we sure we have test coverage for it?

Without this, any contrib or custom code will need to independently determine the requirements for any routes they override.
Also, our current routing system seems to work by coincidence, and without this hardening, route matching could break in the future.

What do you mean by this? RouteProvider::getRoutesByPath() returns matched routes ordered by the 'fit' database column, which favors exact path parts over variable slugs. So why is this "by coincidence" and what prevents contrib/custom code from trusting the preference order returned by it? Note that I think this patch is very sensible regardless, but just trying to understand how it's a contrib blocker.

I also wondered about how to document this element of the route definition. @tim.plunkett said that eventually most entity types should probably use EntityRouteProviderInterface instead of needing to define *.routing.yml entries. But in the interim, it should be discoverable somehow what these mean.

Symfony documents it in http://symfony.com/doc/current/book/routing.html#adding-requirements.

Maybe just a note on https://www.drupal.org/node/2092643 then?

entity.comment.canonical:
   path: '/comment/{comment}'
@@ -41,6 +43,7 @@ entity.comment.canonical:
     _controller: '\Drupal\comment\Controller\CommentController::commentPermalink'
   requirements:
     _entity_access: 'comment.view'
+    comment: \d+

The path has {comment} in it, so why can't we figure out when building the router that {comment} needs an integer?

It's a bit odd specifying this in places like the book printer friendly page.

I don't really think it's consistent - there's a difference between defining an entity route (which we might automate away for core entities too), and something like book which is adding its own tab. The latter can't be automated away from routing.yml.

However this is just adding information to the route definition, so we could look at doing that centrally or removing it again later if we find a better way to do it.

Also issue title doesn't match the patch, this is more what the patch is actually doing.

And this could go into a patch release as far as I can see - no API addition/deprecation at all.

This should be done everywhere there is a potential for conflict.

Is there anywhere this is done which is not using the entity paramconverter?

db log could potentially run into it, if someone added /admin/reports/dblog/event/foo (which any contrib module could do), but that's not touched here.

Also how does book module know that the print needs an integer entity ID - the only convention here is that these are actually entity IDs in the path and we know that those are ints, there is no other indication that would ever lead you to add that requirement. Page manager might not care that they're entities, people writing route definitions certainly will.

However this is just adding information to the route definition, so we could look at doing that centrally or removing it again later if we find a better way to do it.

Exactly. I think getting this in before RC4 is better than not, as it gets the route definitions to have a registered requirement for what is currently only an implicit requirement. But, I like catch's idea of centralizing where the requirement gets registered, instead of making every route do it, and I don't see any reason such centralization couldn't be committed in a patch release.

Therefore, pushed to 8.0.x.

Note: This issue requires people to rebuild the container, just saying, see #2614866: Update rc3 to rc4 fails

#2507509: Service changes should not result in fatal errors between patch or minor releases should have completely fixed that for tagged core releases.