EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available

Here is some supporting evidence:

core/modules/rest/src/Tests/NodeTest.php
65:    $this->httpRequest('node/' . $node->id(), 'GET', NULL, 'application/json');
87:    $this->httpRequest('node/' . $node->id(), 'PATCH', $serialized, $this->defaultMimeType);
core/modules/rest/src/Tests/UpdateTest.php
140:    $this->httpRequest($entity_type . '/9999', 'PATCH', $serialized, $this->defaultMimeType);
core/modules/rest/src/Tests/DeleteTest.php
62:      $response = $this->httpRequest($entity_type . '/9999', 'DELETE');

Indeed it looks this is correct. However the patch missed two:

core/modules/rest/src/Tests/DeleteTest.php
79:    $this->httpRequest('entity/user/' . $account->id(), 'DELETE');
core/modules/rest/src/Tests/CreateTest.php
123:    $this->httpRequest('entity/entity_test', 'POST', $serialized, $this->defaultMimeType);

I do not know the generic answer to that but at least user DELETE seems wrong since #4 found a rest route for user/{user} . I would say let's fix that one at least. I absolutely have no idea how or where rest routing is generated as I generally try to stay away from routing. (And in fact, I am out of this issue too.)

file has now routes in core, so it falls back to the default, that makes sense.

Only question is if the default should not include the leading entity/ now. file is an interesting example. because contrib adds file routes/links, so that would change the REST routes...

#7 Fails because it only patches the setPattern and did not run your patch in https://www.drupal.org/files/issues/rest-post-canonical.patch. Provide a single patch?

Rerolled

Rerolled

Tagging with API change, as the URLs are now some form of API.

Rerolled

#20 applies to HEAD. Removing the "Needs reroll" tag.

This one should pass now.

@clemens.tolboom:
If we remove it, then calling
Url::fromRoute('rest.entity.' . $this->testEntityType . '.POST')->setAbsolute()->toString()
throws an exception :
exception 'Symfony\Component\Routing\Exception\MissingMandatoryParametersException' with message 'Some mandatory parameters are missing ("entity_test") to generate a URL for route "rest.entity.entity_test.POST".' in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 174

@clemens.tolboom
yes, I did manual testing as well. So I guess the patch looks good now?

In case we do that, we should hae some form of change record, given that this is kinda some API change isn't it? I think the REST interface is considered as real API

This would be a pretty heavy API change at this point. We left out the creation POST route on purpose, because it does not align with any other route that serves HTML (the node creation form for example is on a completely different path).

So we need a pretty strong reason in the issue summary why we should change this at this point, can anyone do that? For backwards compatibility we could register 2 routes that do the same thing, one on /node and the legacy one on /entity/node.

I would close this as won't fix, let's ask Crell, the second rest.module maintainer what he thinks.

I would close this as won't fix, let's ask Crell, the second rest.module maintainer what he thinks.

Now a couple of months later there is even less reasons for that. I guess we just have to say no and maybe think about it in D9? Providing some alias as well seems a bit weird.

That's almost exactly what I first wanted to post. :)

Then I deleted what I wrote. Because this *is* very weird, a Drupal WTF. It's very counterintuitive. That's why I think an alias would actually be a good thing.

Agreed.

I would even argue at that time the sane option should be the default, and the BC layer should be the alias.

Rebased #23, it no longer applied.

A regular URL alias won't work AFAIK. I think we can solve this using an inbound path processor? Something like this? Ideally, we'd even only conditionally enable this, i.e. only for sites that were originally installed using 8.0.x.

A regular URL alias won't work AFAIK. I think we can solve this using an inbound path processor? Something like this? Ideally, we'd even only conditionally enable this, i.e. only for sites that were originally installed using 8.0.x.

Yeah, we could have a container parameter to turn off this behaviour. This sounds like a great idea

Patch is against 8.1. This should happen in 8.1 anyway, otherwise it's a BC break.

Re-tested, but no idea if it'll now pick 8.1. May need to reupload.

Also, will need a CR.

Yet another person who ran into this problem and found it very confusing: #2667186: Can't make a post with cookie authentication.

Clearer title.

Let's see whether this works.

Nice! This looks great :)

1. I'm not seeing a service definition for this new class, so I don't think this can work? You probably have that locally, but forgot to include it in the patch?

2. +++ b/core/modules/rest/src/PathProcessor/PathProcessorEntityResourceBC.php
   @@ -0,0 +1,50 @@
   +    // @todo What happens on language prefixes?
   

   Yeah, I was thinking the same. We definitely need test coverage for that.

   Path processors run in a certain order, so we need to make sure that this runs after the language one.

3. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -93,7 +93,7 @@ public function testCreateWithoutPermission() {
   +    $this->httpRequest($entity_type, 'POST', $serialized, $this->defaultMimeType);
   
   @@ -138,7 +138,7 @@ public function testCreateEntityTest() {
   +      $this->httpRequest($entity_type, 'POST', $serialized, $this->defaultMimeType);
   
   @@ -187,7 +187,7 @@ public function testCreateNode() {
   +        $this->httpRequest($entity_type, 'POST', $serialized, $this->defaultMimeType);
   
   @@ -406,7 +406,7 @@ public function assertReadEntityIdFromHeaderAndDb($entity_type, EntityInterface
   +    $this->httpRequest($entity_type, 'POST', 'kaboom!', $this->defaultMimeType);
   
   @@ -417,7 +417,7 @@ public function assertCreateEntityInvalidData($entity_type) {
   +    $this->httpRequest($entity_type, 'POST', NULL, $this->defaultMimeType);
   
   @@ -436,7 +436,7 @@ public function assertCreateEntityInvalidSerialized(EntityInterface $entity, $en
   +    $response = $this->httpRequest($entity_type, 'POST', $invalid_serialized, $this->defaultMimeType);
   
   @@ -456,7 +456,7 @@ public function assertCreateEntityInvalidSerialized(EntityInterface $entity, $en
   +    $this->httpRequest($entity_type, 'POST', $serialized, $this->defaultMimeType);
   

   We have a leading slash in the other places, so let's do that here too?

This is the interdiff

LOL, apparently I rolled this patch previously. I'll fix my own remarks then :P

Yeah, I was thinking the same. We definitely need test coverage for that.

Path processors run in a certain order, so we need to make sure that this runs after the language one.

Actually, we don't need to do anything. Precisely since the language prefix needs to processed before everything else, the priority of the language path processor is very high. So it's done first. Precisely to allow contrib modules to not have to think about this. So, we can keep the default priority, by not specifying any priority, which gets us priority 0.

Fixed all my feedback in #47.

Looks pretty sane now.

One issue I found is that both \Drupal\views\Plugin\views\display\PathPluginBase::alterRoutes and \Drupal\page_manager\Routing\PageManagerRoutes::alterRoutes gives a damn about HTTP methods, so they potentially override the POST route now.
I think this is simply never the case at the moment, because the default canonical routes are registered first.

Note: the patch in #2449143: REST views specify HTML as a possible request format, so if there is a "regular" HTML view on the same path, it will serve JSON fixes the failures.

Oh, great! Postponing this on that issue (#2449143: REST views specify HTML as a possible request format, so if there is a "regular" HTML view on the same path, it will serve JSON) then.

Added a new issue: #2730497: REST Views override existing REST routes

Superb! I reviewed (and RTBC'd) that one :)

This should pass now.

Random migrate fail:

fail: [Other] Line 139 of core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php:

Retesting.

I just checked page_manager and it indeed had a similar problem than node module. Given its an existing bug in contrib I'm not sure we should care about it: #2736385: Page Manager routing is overly complex and brittle: breaks REST, active trail, fallback pages

#62: I think you wanted to post this on #2730497: REST Views override existing REST routes, not this issue?

@Wim Leers
No, I'm actively reporting it here. There could be a chance that some people used page manager for /node, so when they do and we commit this patch, their sites would be broken.

Ah, ok, thanks for the clarification!

I rerolled this patch thrice:

1. #35: straight reroll, rebased
2. #37: added an initial BC layer, which @dawehner then completely redid in #46
3. #50: address nitpicks

Clearly, I only played a tiny role in writing the actual patch. So I think it's fine for me to RTBC this issue.

I think the patch is ready. Still to be done: issue summary update & change record. I did both of those things (CR: https://www.drupal.org/node/2737401). I also updated the existing documentation (https://www.drupal.org/node/2098511/revisions/view/9709507/9730869). Let's get this in!

Except… there is no test coverage yet to prove that /entity/{entity_type} still works. So we need a small test proving that still works, then this is ready.

Well, I still think we should consider to fix page_manager first. Just imagine /user etc. to be broken on actual sites. There is certainly a non null probability for it.

Good point, let's add the tests

For me this seems to be ready to role, anyone wants to object ;)

It looks perfect:

1. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -120,10 +120,12 @@ public function testCreateEntityTest() {
   +      foreach ([NULL, '/entity/entity_test'] as $path) {
   

   Tests new URL and BC for POSTing entities of type entity_test .

2. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -200,12 +201,14 @@ public function testCreateNode() {
   +      foreach ([NULL, '/entity/node'] as $path) {
   

   Tests new URL and BC for POSTing entities of type node .

3. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -252,11 +255,13 @@ protected function testCreateComment() {
   +    foreach ([NULL, '/entity/comment'] as $path) {
   

   Tests new URL and BC for POSTing entities of type comment .

4. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -298,12 +303,14 @@ public function testCreateUser() {
   +      foreach ([NULL, '/entity/user'] as $path) {
   

   Tests new URL and BC for POSTing entities of type user .

Yay, let's finally get this in :)

It looks like this needs a reroll now that #2724823: EntityResource: read-only (GET) support for configuration entities has landed:

fail: [Other] Line 92 of core/modules/rest/src/Tests/ReadTest.php:
Response message is correct.
Value '{"message":"The \\u0022config_test\\u0022 parameter was not converted for the path \\u0022\\/entity\\/config_test\\/{config_test}\\u0022 (route name: \\u0022rest.entity.config_test.GET.hal_json\\u0022)"}' is identical to value '{"message":"The \\u0022config_test\\u0022 parameter was not converted for the path \\u0022\\/config_test\\/{config_test}\\u0022 (route name: \\u0022rest.entity.config_test.GET.hal_json\\u0022)"}'.

fail: [Other] Line 92 of core/modules/rest/src/Tests/ReadTest.php:
Response message is correct.
Value '{"message":"The \\u0022taxonomy_vocabulary\\u0022 parameter was not converted for the path \\u0022\\/entity\\/taxonomy_vocabulary\\/{taxonomy_vocabulary}\\u0022 (route name: \\u0022rest.entity.taxonomy_vocabulary.GET.hal_json\\u0022)"}' is identical to value '{"message":"The \\u0022taxonomy_vocabulary\\u0022 parameter was not converted for the path \\u0022\\/taxonomy_vocabulary\\/{taxonomy_vocabulary}\\u0022 (route name: \\u0022rest.entity.taxonomy_vocabulary.GET.hal_json\\u0022)"}'.

fail: [Other] Line 92 of core/modules/rest/src/Tests/ReadTest.php:
Response message is correct.
Value '{"message":"The \\u0022block\\u0022 parameter was not converted for the path \\u0022\\/entity\\/block\\/{block}\\u0022 (route name: \\u0022rest.entity.block.GET.hal_json\\u0022)"}' is identical to value '{"message":"The \\u0022block\\u0022 parameter was not converted for the path \\u0022\\/block\\/{block}\\u0022 (route name: \\u0022rest.entity.block.GET.hal_json\\u0022)"}'.

fail: [Other] Line 92 of core/modules/rest/src/Tests/ReadTest.php:
Response message is correct.
Value '{"message":"The \\u0022user_role\\u0022 parameter was not converted for the path \\u0022\\/entity\\/user_role\\/{user_role}\\u0022 (route name: \\u0022rest.entity.user_role.GET.hal_json\\u0022)"}' is identical to value '{"message":"The \\u0022user_role\\u0022 parameter was not converted for the path \\u0022\\/user_role\\/{user_role}\\u0022 (route name: \\u0022rest.entity.user_role.GET.hal_json\\u0022)"}'.

Those are specifically referring to the config entities that can now be POSTed. The path processor needs to only strip the leading /entity if the entity type has a canonical path.

Wonderful, thanks!

What does this mean for

Since most configuration entities do not canonical links they will be available at the path 'entity/[ENTITY_TYPE]/[ENTITY_ID]'.
For example to retrieve the tags vocabulary use.
curl --user admin:admin --request GET "http://drupal.d8/entity/taxonomy_vocabulary/tags?_format=json"

From #2724823: EntityResource: read-only (GET) support for configuration entities?

As the patch shows: /<config entity type>/<entity ID>.

@Wim Leers but doesn't that conflict with

The path processor needs to only strip the leading /entity if the entity type has a canonical path.

Let me take a deeper look at this tomorrow, so I can formulate a proper answer.

+++ b/core/modules/rest/src/Plugin/Deriver/EntityDeriver.php
@@ -73,8 +73,8 @@ public function getDerivativeDefinitions($base_plugin_definition) {
         $default_uris = array(
-          'canonical' => "/entity/$entity_type_id/" . '{' . $entity_type_id . '}',
-          'https://www.drupal.org/link-relations/create' => "/entity/$entity_type_id",
+          'canonical' => "/$entity_type_id/" . '{' . $entity_type_id . '}',
+          'https://www.drupal.org/link-relations/create' => "/$entity_type_id",
         );

I would indeed argue that we have an out of scope change here. The default canonical aka. fallback path for entities is /entity/{$entity_type_id}, so config entities are currently exposed under /entity/config_test/{config_test} for example.
We introduce a BC break in a way, or at least a change which is not part of the problem in most cases.

One could say \Drupal\rest\Tests\ReadTest::getReadUrl is a sign of a too abstracted test, as its not entirely obvious what is going on.

This was just a GET URL involved

The patch in #73 and all prior patches changed the default canonical URI path in EntityResource. That URI path was used if the entity type didn't specify a specific canonical URI path. Technically it is indeed out of scope to do that, and from that POV #79 and #81 are correct.

However… in doing this, AFAICT this will start to break in #2300677: JSON:API POST/PATCH support for fully validatable config entities: because PathProcessorEntityResourceBC always strips the /entity prefix for POST requests, you won't be able to HTTP POST /entity/taxonomy_vocabulary, for example: the inbound path processor that this patch will always rewrite that to HTTP POST /taxonomy_vocabulary, which points to a non-existing path.

I'd argue that it is in the spirit of this patch to change all entity REST URIs to use /{entity_type}/{entity_id}, and therefore to change the default/fallback canonical entity resource URI paths. In which case the previous patch, i.e. the one in #73, is the appropriate one to commit.

In doing so, we'd have:

1. consistent behavior across all entity types
2. no breakage for content entity types without a canonical URI path (such as File)
3. no breakage for config entity types when they gain the ability to be POSTed to the REST resource (i.e. in #2300677: JSON:API POST/PATCH support for fully validatable config entities

Technically it was decided in #2019123: Use the same canonical URI paths as for HTML routes to do so, so sure, let's rename the issue title then.

We agreed on changing it

UpdatePathWithBrokenRoutingFilledTest failed. Let's see if it was a random fail or whether HEAD is broken.

Yeah at that point in time basically every update test is failing randomly.

+++ b/core/modules/rest/src/PathProcessor/PathProcessorEntityResourceBC.php
@@ -0,0 +1,51 @@
+    if ($request->getMethod() === 'POST' && strpos($path, '/entity/') === 0) {

+++ b/core/modules/rest/src/Plugin/Deriver/EntityDeriver.php
@@ -73,8 +73,8 @@ public function getDerivativeDefinitions($base_plugin_definition) {
-          'canonical' => "/entity/$entity_type_id/" . '{' . $entity_type_id . '}',
-          'https://www.drupal.org/link-relations/create' => "/entity/$entity_type_id",
+          'canonical' => "/$entity_type_id/" . '{' . $entity_type_id . '}',
+          'https://www.drupal.org/link-relations/create' => "/$entity_type_id",

Now we've changed this issue to also change the default for canonical - we'd need to also fix GETs but that seems super risky.

What @alexpott and myself talked about is the following:

• Don't change the default, but rather change it for the entity types in core. This results in no BC break for other entity types
• Change the BC layer to not just use the /entity prefix but also really just change /entity/\d+
• Another idea I had in mind, store the regexes of those rest routes and use that in the path processor. This ensure that we just convert the examples, that has to be converted

All in all this would result in a more minimal BC break.

#90: Damn, good point :/

#91:

This results in no BC break for other entity types

But we want custom/contrib entity types to also post to /<entity type>, not to /entity/<entity type>. We want that to still be consistent, so we should still do it for all entity types that do specify a canonical URL.

So, I agree with everything in #91, except for one thing: but rather change it for the entity types in core. — we shouldn't do that, we should do it for all entity types that do specify a canonical URL in my opinion.

But we want custom/contrib entity types to also post to /, not to /entity/. We want that to still be consistent, so we should still do it for all entity types that do specify a canonical URL.

There is no reason that they cannot do that. They can add the link template. Risking BC when we don't have to, is IMHO an antipattern.

They can add the link template.

Ahh! Now I understand.

So they'd each have to do something like this:

/**
 * Defines the node entity class.
 *
 * @ContentEntityType(
 *   id = "node",
 *   label = @Translation("Content"),
…
 *   links = {
 *     "canonical" = "/node/{node}",
 *     "delete-form" = "/node/{node}/delete",
 *     "edit-form" = "/node/{node}/edit",
 *     "version-history" = "/node/{node}/revisions",
 *     "revision" = "/node/{node}/revisions/{node_revision}/view",
… 
 *     "https://www.drupal.org/link-relations/create" = "/node",
 *   }
 * )
 */

(Look at that very last line in the annotation.)

That makes sense.

+1

@Wim Leers Exactly, do you think less risk is better here?

Yes, +1.

Actually, @EclipseGc made an excellent remark yesterday in personal chat. Why are we not simply using the collection link relation? Creating a new entity of a certain type is the same as POSTing something to a collection. That makes perfect sense. It'd also mean we can deprecate the silly https://www.drupal.org/link-relations/create link relationship. https://en.wikipedia.org/wiki/Representational_state_transfer#Relationsh... also explicitly mentions this.

So I did some digging. Apparently #2019123: Use the same canonical URI paths as for HTML routes introduced the https://www.drupal.org/link-relations/create link relationship. Despite that issue being done by resident REST experts @klausi and @Crell, there were zero mentions of collection (well, there were mentions of that particular word in the context of a route collection, but the link relationship was not mentioned once).

Now, the funny thing is that we have 14 entity types in core that specify a collection relationship. But node isn't one of them. Nor is comment.

So, it looks like in order to do what is really right, we'll first have to fix the link relations on many entity types.

Thoughts?

The "every resource type has one definitive collection and you POST to that to create something" is not, AFAIK, part of REST style as defined by Fielding. It's a common pattern seen in demo applications and other cases where "All the Thingies" is the only reasonable collection of Thingies, but that's not at all universal. In fact, I have built systems for clients where we had /thingie/1/stuff/4 as a "stuff" resource, and the "collection" was therefore /thingie/1/stuff, and that's where you'd POST. There was also /thingie/8/stuff if you wanted to add things to THAT collection. (In this case, it was a collection of stuff that belonged to that particular thingie.)

Thus, a blanket statement that all entities are primarily a part of the "all the thingies" collection, and that is their primary existence is, I would argue, incorrect. That's why we didn't do that. Arguably we could make the assumption that all comments are first and foremost part of the node/X/comment collection (or whatever other base entity they are on, since in D8 comments are not node-specific), but I don't think any such assumption could be made for nodes.

Wim, you don't mention which those 14 entities are that have a collection relationship, but I'll venture a guess that many/most are config entities, or other entities with very low cardinality and relationship complexity. In those cases the simplified default assumption probably dos hold. But that's not the universal REST Way, just a common degenerate case, and so not an assumption we should bake into the whole system.

That is, "Creating a new entity of a certain type is the same as POSTing something to a collection" is only true sometimes. It is not a universal.

That might be complete bogus in a REST world, but we currently use collection routes for entity list builders. The fact that not very many entity types actually provide link templates for the collection link templates is owed to the fact that we are not capable of auto-generating a list-builder-route yet (!). Once we can do that, we will add collection link templates, but they will point to paths like /admin/content and /admin/config/user-interface/shortcut. While - again - that might make the stomache of someone coming from a REST POV twist, I don't think that's something we can change at this point.

Might also be that this is completely beside the issue and I'm just rambling, in which case sorry and carry on, but the mention of the collection relation above made me stutter for a moment.

#98: thanks a lot for that comment, that was very interesting!

#99: indeed, many of the collection routes are admin-UI-specific URLs, which makes them… less than ideal for REST purposes. However, I'm not sure I understand your conclusion: are you saying you're agreeing with #97, or that you were frustrated by the explanation in #98, or …?

@Crell

So, while I don't have a problem with your basic argument, your example does nothing to convince me. That's still defining the collection of the stuff entity as /thingie/{thingie}/stuff. It is then perfectly logical that posting to that will make a stuff entity related to the thingie in question. This is EXACTLY how I'd want comments (and taxonomy terms) to behave. Of course none of this has to be forcefully thrust upon all entity types, but as a guiding principle, I think it's a pretty good (and common) one. No?

Eclipse

Well, it's true in many cases, certainly. But to take comments, we support comments on nodes, on users, on commerce products, etc. Technically, we even support multiple comment fields/threads on a single entity. (I think? I've not actually tried and I don't know why you would, but isn't that a side effect of comment-config-as-field?) That means simply defining the comment "collection" route as /node/{node}/comments isn't sufficient, because comments would also make sense at /user/{user}/comments, /node/{node}/internal_comments (alternate field), /commerce/product/{product}/comments, etc.

That is, simply standardizing all entities to a "collection" route, in their annotation, of /{entity_type} is not going to work. That would be true in some cases, perhaps a majority, but would break badly in many. We would need to at least allow for collection to be defined contextually at runtime instead.

While I think the discussion around the collection link relation is worth to do, I think this issue is about a bug in REST itself and its inconsistency. Fixing that is a small iteration step, vs. what we discuss which is a big step compared to it. Its totally worth to do, but it should be its own thing.

Regarding the collections, I think those collections should be created, but in order to design properly, one needs domain knowledge, rest module cannot have. REST module made the tradeoff early that its primarily a generic export all the things mechanism, rather something well tuned like a RESTAPI of github for example.

I only brought the collections thing up because - per this issue's title - I thought the intent was to make the "collection" link in the entity annotation point to e.g. "/node". That would be a pretty big conceptual (and API) change, which I tried to explain above. If that is in fact not the case, then this can be ignored. I'm not sure reading the latest comments what the intent of the issue is, TBH.

So let's get this issue back on track! What still needs to happen for this issue to go back to RTBC?

1. In #90, Alex Pott un-RTBC'd it.
2. In #91–#96, the necessary changes were discussed and fully agreed upon.
3. Then, in #97, I posted a comment that basically proposed a very different direction that @EclipseGc told me about: using the collection link relationship's URI.
4. In #98, Crell explained that EclipseGc rationale is indeed very common, but not universally applicable. And hence it is an assumption we should not bake into the system.
5. in #101 and #102 Crell and EclipseGc continue the discussion.
6. In #103, dawehner argues that assuming that we can use collection, then it'd still be inappropriate to do so here, because it requires many more changes.

So, basically, #97 through #104 was a big, distracting What if? that would lead to a different direction for the patch. And it'd be very valuable, if it turns out to be possible.
But this issue is just about fixing the WTF that is GET /node/42, PATCH /node/42, DELETE /node/42 versus POST /entity/node. That should just be POST /node (every person who's used REST so far has found that unintuitive/strange). That's all this is about. And that's something we can do without the even bigger changes proposed in #97–#104.

Therefore let us go back to #90 + #91–#96, where we were so close to a patch that resolves this big REST Developer Experience WTF.

This implements everything discussed in #90–#97. CR updated as well: https://www.drupal.org/node/2737401/revisions/view/9730871/9919603.

I think this idea in #91 would be a splendid minor performance improvement for a follow-up:

Another idea I had in mind, store the regexes of those rest routes and use that in the path processor. This ensure that we just convert the examples, that has to be converted

But for now, I've gone with the simplest possible approach that works, so that this can still go in before feature freeze next week.

Already updated the patch, CR and IS to reflect the new direction. Now also updating the title.

This is so much better. Thanks, @alexpott and @dawehner!

1. +++ b/core/modules/rest/src/PathProcessor/PathProcessorEntityResourceBC.php
   @@ -0,0 +1,51 @@
   +    if ($request->getMethod() === 'POST' && strpos($path, '/entity/') === 0) {
   +      $parts = explode('/', $path);
   +      $entity_type = array_pop($parts);
   

   Can we ensure to just match on /entity/{entity_type} and nothing more?

2. +++ b/core/modules/taxonomy/src/Entity/Term.php
   @@ -46,6 +46,7 @@
   + *     "https://www.drupal.org/link-relations/create" = "/taxonomy_term",
   

   IMHO this path is a bit weird. Don't we want /taxonomy/term instead?

1. That's what the code after that does? Hm, I see it's not strict enough. Let me fix that.
2. Because it's GET|PATCH|DELETE /taxonomy_term/42 already. It'd be much more confusing to have GET /taxonomy_term/42, but POST /taxonomy/term

Hm, I see it's not strict enough. Let me fix that.

Yeah this was kinda the point alexpott and I talked about in #90 and #91

Well, GET is certainly on "/taxonomy/term/{taxonomy_term}" but yeah we some some inconsistency there. nevermind.

So yeah please ignore me little rambling there :)

These test failures are kind of interesting to be honest.
They are basically really related with #2113345: Define a mechanism for custom link relationships as in we have, names for our link relations, and external URLS where they are defined.

[28-Jul-2016 23:07:40 Australia/Sydney] Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "entity.node.https://www.drupal.org/link_relations/create" does not exist." at /var/www/html/core/lib/Drupal/Core/Routing/RouteProvider.php line 187

In case we would have #2113345: Define a mechanism for custom link relationships in, there would be also a way to use "create" in the link templates, but then still expose the right link relation to the public.

Is possible to fix without #2113345: Define a mechanism for custom link relationships?

Classes like \Drupal\node\Controller\NodeViewController assume call \Drupal\Core\Entity\Entity::url which will call \Drupal\Core\Entity\Entity::toUrl

Entity::toUrl assumes it can make a route name with $rel.

$route_name = "entity.{$this->entityTypeId}." . str_replace(array('-', 'drupal:'), array('_', ''), $rel);
      $uri = new Url($route_name, $route_parameters);

That is why you get the error in #112.

Also on a related note should we actually be updating Entity class files like Node.php?

Isn't
"https://www.drupal.org/link-relations/create" = "/node",
Only valid if the REST module enabled or maybe I am missing something?

Should we be using hook_entity_type_alter to add this links to the entity definition? This would probably fix a lot of the test because they won't turn on the REST module but it won't fix the underlying problem.

Yeah let's be honest, this gives us the abstractions we need

#2113345: Define a mechanism for custom link relationships is FINALLY in, so now this can continue! We should still try to get this in 8.3.

I will reroll this first thing in the morning unless somebody beats me to it.

First, a straight reroll.

#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method completely changed the test coverage, and thus those portions no longer apply: core/modules/rest/src/Tests/CreateTest.php and core/modules/rest/src/Tests/ReadTest.php no longer exist. This means we must still do the work to restore the removed test coverage! (I've attached an interdiff that shows which parts of the patch I removed. It's not actually an interdiff, it's the portion of #105 that I've literally cut away, because it cannot apply at all anymore.)

I expect this to have a lot of fails.

+++ b/core/modules/rest/src/PathProcessor/PathProcessorEntityResourceBC.php
@@ -0,0 +1,51 @@
+      if ($this->entityTypeManager->getDefinition($entity_type)->hasLinkTemplate('https://www.drupal.org/link-relations/create')) {
+        // Return the path minus the leading '/entity'.
+        return substr($path, 7);

+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -46,6 +46,7 @@
+ *     "https://www.drupal.org/link-relations/create" = "/taxonomy_term",

Couldn't we return $entity_type->getLinkTemplate(...) instead of always using the entity type ID? I.e. for taxonomy terms maybe we want to use /taxonomy/term?

Edit: I see that that has been discussed in #108 and following, but it still seems that in general this should be configurable and we should use the provided link template?

#120: I think you're right :) That definitely needs to be done before this can be RTBC. But first, bigger worries: getting this patch to green. Details about the trickiness to follow…

#2113345: Define a mechanism for custom link relationships added link relations, but did so with https://drupal.org/link-relations/… URIs instead of https://www.drupal.org/link-relations/… which was used in this patch. Rather than modifying core.link_relation_types.yml, I think it's better to modify this patch.

Not only that, but it's probably better/more accurate to use the key specified in core.link_type_relations.yml: i.e. use create, not https://drupal.org/link-relations/create. Just like entity type annotations.

This made me realize: then how can you discern registered link relation types versus extension link relation types?! We should use identify them by their name and URI respectively. That'd match the spec. So we should continue to do it like this patch is already doing: using URIs!

However… that won't work because Drupal 8 shipped with violations of this principle already, and we can't change that because it'd break BC. We should have done #2113345: Define a mechanism for custom link relationships before D8 shipped, then we wouldn't have made this mistake. For example for Node in HEAD:

 *   links = {
 *     "canonical" = "/node/{node}",
 *     "delete-form" = "/node/{node}/delete",
 *     "edit-form" = "/node/{node}/edit",
 *     "version-history" = "/node/{node}/revisions",
 *     "revision" = "/node/{node}/revisions/{node_revision}/view",
 *   }

Of those, only canonical, edit-form and version-history are registered link relation types (i.e. registered with and accepted by the IANA). So that means delete-form and revision are already extension link relation types (i.e. Drupal-specific).

(This also means that if the IANA accepts new link relation types that happen to use the same names as the ones Drupal used, but with a different meaning, that we will be forced to break BC. For D9, we should change the entity type links annotation to use URIs as keys for extension link relation types, and we should also make link_relation_types.yml use URIs as keys for those.)

So the existing entity type annotations (and associated logic) requires us to not use URIs. But OTOH the REST module's logic requires us to use URIs. There's no way to reconcile these. We must break one of the two!

I think there is one of those that is clearly more important (and would cause a bigger disruption): the entity type annotation. Very few @RestResource plugins have ever figured out how to define a custom deriver. Plus we can add a BC layer in \Drupal\rest\Plugin\ResourceBase::routes(), which means we can actually guarantee zero disruption. Whew!

Ironically, this (the Node entity type annotation cited above) was introduced by #1970360: Entities should define URI templates and standard links, which itself was all about using link relation types + associated link templates for REST… that enabled #2019123: Use the same canonical URI paths as for HTML routes, which is what made the REST module actually use entity type-defined link templates. It already pointed to #2113345: Define a mechanism for custom link relationships as the place to settle on a final solution, but said it should not be blocked on that issue. That was a huge mistake. That's why we have this mess. That's why this issue is still not solved, more than 3.5 years after #2019123 was opened.
In fact, @linclark predicted that the logic in the REST module would have to change depending on how #2113345: Define a mechanism for custom link relationships would be solved. See her comment at #2019123-43: Use the same canonical URI paths as for HTML routes. Also, it's that same issue (#2019123) that made bold claims about how much better REST URLs became (see the change record), but it also forgot to mention the fact that it only did so for GET/PATCH/DELETE, it did not make the POST case better. Which is what we're still trying to solve here, almost 3 years after that was committed.

Therefore, we have but one choice:

+++ b/core/modules/aggregator/src/Entity/Feed.php
@@ -33,6 +33,7 @@
+ *     "https://www.drupal.org/link-relations/create" = "/aggregator/sources",

+++ b/core/modules/block_content/src/Entity/BlockContent.php
@@ -41,6 +41,7 @@
+ *     "https://www.drupal.org/link-relations/create" = "/block",

Change hunks like these to use "create" = "…" instead. Did that.

This causes some disruption, but we can mitigate it completely by doing Plus we can add a BC layer in \Drupal\rest\Plugin\ResourceBase::routes(), which means we can actually guarantee zero disruption. — did that too.

As you can tell, the test results of #124 are littered with PHP errors like this:

Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "entity.node.create" does not exist." at /var/www/html/core/lib/Drupal/Core/Routing/RouteProvider.php line 187

This is because \Drupal\node\Controller\NodeViewController::view() (soon hopefully EntityViewController — see #2282029: Automate link relationship headers for all entity types, not just nodes) and \Drupal\rest\Plugin\rest\resource\EntityResource::addLinkHeaders() call $node->toUrl('create') (which is correct btw). That in turn tries to generate the URL for the entity.node.create route. Which… fails. Because it doesn't exist.

It doesn't exist, because despite create being a link relation type specifically for REST, \Drupal\rest\Plugin\ResourceBase::routes() always wants to create routes with names like rest.entity.node.POST.

So this breaks both REST responses and HTML responses for /node/NID, with the fatal error mentioned above.

We could make all code that iterates over link templates should support the fact that a route may not be defined. Then the create link would never be listed, but at least it wouldn't cause fatal errors.

Let's at least do that for now, that should reduce the number of failures.

So #126 was not a full solution. We are not yet able to generate create link headers, because the necessary route does not exist.

Symfony does not support the concept of "route aliases" or "route pointers". We'd have to have rest.entity.node.POST and entity.node.create. And they'd have to have identical route definitions. That's the only way to make two route names point to the same route.

We can't drop rest.entity.node.POST either, doing so might break BC. Nor can we drop entity.node.create, because that would break both \Drupal\Core\Entity\Entity::toUrl() and the entity type link templates API.

Letting \Drupal\rest\Plugin\rest\resource\EntityResource::routes() generate a entity.node.create route with such a duplicate definition doesn't work, because \Drupal\rest\Routing\ResourceRoutes::alterRoutes() prefixes all routes with 'rest'.
I can make that work by also modifying ResourceRoutes::alterRoutes() to no longer set the rest. prefix on route names, and just setting that prefix in EntityResource and DBLogResource. But this would break BC for other REST resources — their route names would change.

That almost leaves me with only one choice: a \Drupal\Core\Routing\RoutingEvents::DYNAMIC event subscriber that iterates over all RestResourceConfig entities, looks which ones are for the EntityResource @RestResource plugin, and adds a entity.<entity type>.create route with the same definition as the rest.entity.<entity type>.POST.
But this feels quite strange IMO.

Modeled after \Drupal\Core\EventSubscriber\EntityRouteProviderSubscriber.

And UGH… in the process I noticed that \Drupal\rest\Routing\ResourceRoutes is using RoutingEvents::ALTER, but it really should have been using RoutingEvents::DYNAMIC! To be fair, that was added in #2158571: Routes added in RouteSubscribers cannot be altered, long after this class was created. But irony oh irony, this very class was mentioned in #2158571 many times as an example of what should use DYNAMIC, but then it didn't do that. So, fixing that too.

Now addressing #120:

Couldn't we return $entity_type->getLinkTemplate(...) instead of always using the entity type ID? I.e. for taxonomy terms maybe we want to use /taxonomy/term?

Fixed both of those things.

Adding back test coverage that was lost in #119. Thanks to the overhauled test coverage, adding test coverage has just become trivial.

Round of self-review.

1. +++ b/core/modules/aggregator/src/Entity/Feed.php
   @@ -33,6 +33,7 @@
   + *     "create" = "/aggregator/sources",
   

   This is a config entity. We shouldn't add this link template for config entities yet.

   (Config entities currently only support GET.)

2. +++ b/core/modules/rest/src/EventSubscriber/EntityResourcePostRouteSubscriber.php
   @@ -0,0 +1,74 @@
   +      // We only care about REST resource config entities for the
   

   s/for/that use/

3. +++ b/core/modules/rest/src/PathProcessor/PathProcessorEntityResourceBC.php
   @@ -0,0 +1,51 @@
   +      // Only remove the '/entity' prefix if we have a matching entity type
   +      // following it. Otherwise, this is a route other than the one for
   +      // \Drupal\rest\Plugin\rest\resource\EntityResource, and hence we don't
   +      // want to change anything.
   +      if ($this->entityTypeManager->getDefinition($entity_type)->getLinkTemplate('create')) {
   +        // Return the path minus the leading '/entity'.
   +        return substr($path, 7);
   +      }
   

   The comment doesn't match the logic. And actually, the logic is very wrong too.

   My changes in #128 were completely wrong. It's been a long day :(

   This means that e.g. for taxonomy terms, this will fail.

4. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
   @@ -100,8 +100,11 @@ public function routes() {
   -    $create_path = isset($definition['uri_paths']['https://www.drupal.org/link-relations/create']) ? $definition['uri_paths']['https://www.drupal.org/link-relations/create'] : '/' . strtr($this->pluginId, ':', '/');
   -
   +    $create_path = isset($definition['uri_paths']['create']) ? $definition['uri_paths']['create'] : '/' . strtr($this->pluginId, ':', '/');
   

   We shouldn't change whitespace.

5. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
   @@ -100,8 +100,11 @@ public function routes() {
   +    // BC.
   

   This needs a better comment.

6. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -369,9 +370,19 @@ public function calculateDependencies() {
   -          ->toString(TRUE);
   +
   +        // It's not guaranteed that every link relation type also has a
   

   Unnecessary whitespace.

7. \Drupal\rest\Plugin\rest\resource\EntityResource still has:
   

    *   uri_paths = {
    *     "canonical" = "/entity/{entity_type}/{entity}",
    *     "https://www.drupal.org/link-relations/create" = "/entity/{entity_type}"
    *   }
   

   That should become create.

8. Should PathProcessorEntityResourceBC be renamed to PathProcessorEntityResourcePostRouteBC?

Addressed everything except the last point.

CR updated.

IS updated.

Sigh, complete DrupalCI failure. We'll have to wait for DrupalCI to work again before we can retest.

#127 through #130 will need to be retested.

1. +++ b/core/modules/comment/src/Entity/Comment.php
   index fce50e1..ba73ba6 100644
   --- a/core/modules/node/src/Controller/NodeViewController.php
   
   +++ b/core/modules/node/src/Controller/NodeViewController.php
   @@ -56,6 +57,17 @@ public function view(EntityInterface $node, $view_mode = 'full', $langcode = NUL
   +      try {
   +        $generated_url = $url->toString();
   +      }
   +      catch (RouteNotFoundException $e) {
   +        continue;
   +      }
   +
   

   Is it preferable here to catch an exception rather than check the route exists?
   something like:

   if (!$url->isRouted() || !\Drupal::service('router.route_provider')->getRoutesByNames([$url->getRouteName()])) {
           continue;
         }

   Probably not but thought I would mention it.

2. +++ b/core/modules/rest/src/EventSubscriber/EntityResourcePostRouteSubscriber.php
   @@ -0,0 +1,74 @@
   +      $rest_post_route = $route_collection->get($rest_post_route_name);
   +
   +      // Create a route for the 'create' link relation type for this entity type
   +      // that uses the same route definition as the REST 'POST' route which use
   +      // that entity type.
   +      // @see \Drupal\Core\Entity\Entity::toUrl()
   +      $entity_create_route_name = "entity.$entity_type_id.create";
   +      $route_collection->add($entity_create_route_name, $rest_post_route);
   

   If a particular entity resource is configured not to enabled POST $rest_post_route will be null and then a fatal error for adding the collection.

   I hit this b/c the site I was debugging had 'file' entity type setup this way.

   Fixed in attached patch.

For the problems I found with @EntityType annotations' links portion, I opened #2848945: EntityType link templates: using link relation type names as keys is a problem.

#138.2: HAH! Yes, thanks :)

#138.1: The current approach works for any URL, yours requires more logic, and ends up doing the same thing. So not a fan of the approach you mentioned at first sight, but also not opposed.

@Wim Leers for #138.2 that sounds like it is fine already the way it is.

1. +++ b/core/modules/node/src/Controller/NodeViewController.php
   @@ -56,6 +57,17 @@ public function view(EntityInterface $node, $view_mode = 'full', $langcode = NUL
   +      try {
   +        $generated_url = $url->toString();
   +      }
   +      catch (RouteNotFoundException $e) {
   +        continue;
   +      }
   

   Another possible exception could be a messing parameter in the generator URL, like when the relation type needs more than one parameter. \Symfony\Component\Routing\Exception\MissingMandatoryParametersException would be thrown in that case.

2. +++ b/core/modules/rest/src/Routing/ResourceRoutes.php
   @@ -54,18 +56,18 @@ public function __construct(ResourcePluginManager $manager, EntityTypeManagerInt
   -  protected function alterRoutes(RouteCollection $collection) {
   +  public function onDynamicRouteEvent(RouteBuildEvent $event) {
   

   +1

#144.1: Right, currently there are no create link templates that use parameters, but that's indeed possible. Fixing that…
Although hm, no, that's not possible. Look at \Drupal\Core\Entity\Entity::toUrl(). It automatically retrieves the necessary route parameters using \Drupal\Core\Entity\Entity::urlRouteParameters(). That already works for all other link relation types. So it will work fine for the create link relation type too. So the code is fine as-is.

Apparently it's taxonomy_page_attachments_alter() that's adding link relation headers for the canonical Term route! We'll need to update that logic just like we updated NodeViewController's.

That will fix some failures.

There are a bunch of these failures:

Uncaught PHP Exception RuntimeException: "Callable "Drupal\Core\Access\CsrfRequestHeaderAccessCheck::access" requires a value for the "$request" argument." at /var/www/html/core/lib/Drupal/Component/Utility/ArgumentsResolver.php line 142

The root cause is simple: that service needs the request object (hence that error), but it's not tagged correctly.

100% of the remaining failures is in User entity type REST tests.

Why? Because thanks to #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available, we have comprehensive test coverage, including of edge cases. The failing assertions are asserting that we're getting an error response when POSTing to /user. But with this patch applied, we're not getting a 4xx response, we're getting a 200! Why? Because /user matches the user.page route:

user.page:
  path: '/user'
  defaults:
    _controller: '\Drupal\user\Controller\UserController::userPage'
    _title: 'My account'
  requirements:
    _user_is_logged_in: 'TRUE'

… which requires the user to be logged in. If the user is not logged in, one is redirected automatically to /user/login. So, Guzzle simply follows this redirect and then repeats the same POST request, but this time to /user/login.

This then matches three routes:

1. user.login (/user/login), which has no _format or _content_type_format requirements
2. user.login.http (/user/login), which has only a _format requirement
3. user.entity.canonical (/user/{user}), which has no _format or _content_type_format requirements

The second one is filtered away by \Drupal\Core\Routing\MethodFilter during route matching. That leaves the first and the third. The first one is selected and since there's no actual request body (i.e. $_POST is empty), UserLoginForm happily returns the form (since an empty request body does not trigger form submission) and hence it results in a 200 response.

Now, it may be easy to blame the form system for sending a 200 response to an empty POST request. But the actual problem lies much earlier: in the redirect. It's the redirect that's causing all these problems. The redirect should never have happened! The redirect should only happen for HTML requests. What identifies HTML requests, is the absence of a ?_format string AND a Content-Type request header that is either:

• absent (if GET request)
• set to application/x-www-form-urlencoded
• set to multipart/form-data

In other words: our pre-D8 days are coming to haunt us, the decision to let D7 menu callbacks become _controller callbacks (first _content) that return render arrays without needing to specify any additional requirements, is actually the problem here.

Changing the defaults (i.e. _content_type_format) set for such routes (see \Drupal\Core\EventSubscriber\RouteMethodSubscriber) is probably a bad idea; it's likely it'll cause some disruption. Plus, multipart/form-data is actually also very commonly used for REST, i.e. for non-HTML routes. So we can't assign that anyway.

I see only two solutions:

1. Change the way EntityResourceTestBase uses Guzzle: make it no longer follow redirects. The 200 will become a 301. This is something we should probably do anyway.

   But it doesn't solve the actual problem: the redirection does not make sense.

2. changing
   

   user.page:
     path: '/user'
     defaults:
       _controller: '\Drupal\user\Controller\UserController::userPage'
       _title: 'My account'
     requirements:
       _user_is_logged_in: 'TRUE'
   

   to:

   user.page:
     path: '/user'
     defaults:
       _controller: '\Drupal\user\Controller\UserController::userPage'
       _title: 'My account'
     requirements:
       _method: 'GET'
       _user_is_logged_in: 'TRUE'
   

   This will reject any POST requests for that route, which is fine, because it's a pure convenience route anyway, that always redirects (to either /user/login or to /user/{user}, depending on whether you're logged in or not).

   The current 200 response (well, the 301 redirect, then 200) will become a 405 response (method not allowed).

   But it doesn't solve the problem generically, it solves the problem only for this particular entity type, because it happens to have this particular route.

So I implemented this solution by implementing both parts. Part 1 is not necessary but helps prevent surprises. Part 2 is essential. It does mean that there is some special casing in EntityResourceBase for User, but it's very very limited.

WTF d.o. I pressed "upload", and you submitted it.

Here are then the files that belong to #150.

Tests were able to get further in the test scenario, but they got stuck on the final bit when posting users.

Here's the last necessary fix, to prevent those 422 responses (due to Unprocessable Entity: validation failed. name: The username Dramallama ic28G5Or is already taken) and get them to be 201 responses again.

In another round of self-review, I can find only one nit:

+++ b/core/modules/rest/rest.services.yml
@@ -35,8 +35,23 @@ services:
+      - { name: path_processor_inbound }
+

Unnecessary blank line.

Hopefully this is now getting close to RTBC :)

The two remaining tests are both Contact-related. Reproduced.

So, this test is failing in ContactSidewideTest: $this->updateContactForm($id, $label = $this->randomMachineName(16), $recipients_str = implode(',', array($recipients[0], $recipients[1])), $reply = $this->randomMachineName(30), FALSE, 'Your message has been sent.', '/user');. Note that it's setting the redirect URL to /user. So now it's validating the /user URL using the PathValidator. And remember that in #150/#151, we configured that route to only allow the GET method to be used.

The root cause: PathValidator reuses the current request. Hence it inherits the POST method if PathValidator is called in a form's submit handler. And hence \Drupal\Core\Render\Element\PathElement::validateMatchedPath() now complains about /user, because of the changes in #150/#151.

Specifically that method calls:

• \Drupal\Core\Path\PathValidator::getUrlIfValid(), which calls
• \Drupal\Core\Path\PathValidator::getUrl(), which does
  

      $request = Request::create('/' . $path);
      $attributes = $this->getPathAttributes($path, $request, $access_check);
  

• \Drupal\Core\Path\PathValidator::getPathAttributes() calls \Drupal\Core\Routing\Router::match(), which calls
• \Drupal\Core\Routing\Router::matchRequest(), which calls
• \Symfony\Component\Routing\Matcher\UrlMatcher::matchCollection(), which does NOT receive the request object, and instead retrieves the method like this:
  

  $method = $this->context->getMethod()
  

  i.e. it reads the request method from the global request context, and hence incorrectly concludes that it's matching a collection for a POST request!

  We need to make sure that we've updated the global request context object (the router.request_context service, actually) to use our fake request object that we use purely for validation purposes.

  Ideally, we should be able to just push our fake request onto the request stack… but sadly Symfony is incredibly poorly architected not taking care of that automatically, leaving us with the request stack and request context getting out of sync… So the solution is to not push onto the request stack, but rather to get the current request from the request stack, set our fake request on the request context, and then restore it onto the request context (because yay, the request context service/object has no way to read all of its information, so we can't read from it to then restore it).

Whew. This is a great showcase of why I proposed https://events.drupal.org/baltimore2017/sessions/maintainability-only-co....

We have this RTBC issue: #2822190: PathValidator validates based on a RequestContext leaked from the current request, resulting in false negatives during CLI requests and POST submissions which fixes exactly that problem.

#158: THANK YOU. I was dreading writing test coverage for this. And it really belongs in a separate issue. Very glad it already exists, and is already RTBC!

Although hm, no, that's not possible

Oh yeah fair. When we run into the problem it would be a problem everywhere else?

1. +++ b/core/core.services.yml
   @@ -1143,7 +1143,7 @@ services:
        class: Drupal\Core\Access\CsrfRequestHeaderAccessCheck
        arguments: ['@session_configuration', '@csrf_token']
        tags:
   -      - { name: access_check }
   +      - { name: access_check, needs_incoming_request: TRUE }
      maintenance_mode:
        class: Drupal\Core\Site\MaintenanceMode
   

   +1

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -497,7 +497,7 @@ public function testPost() {
        if ($has_canonical_url) {
   -      $this->assertSame(404, $response->getStatusCode());
   +      $this->assertSame(static::$entityTypeId === 'user' ? 405 : 404, $response->getStatusCode());
   
   @@ -510,7 +510,12 @@ public function testPost() {
   +    if (static::$entityTypeId === 'user') {
   +      $this->assertResourceErrorResponse(405, 'No route found for "POST ' . str_replace($this->baseUrl, '', $this->getPostUrl()->setAbsolute()->toString()) . '": Method Not Allowed (Allow: GET, HEAD)', $response);
   +    }
   +    else {
   +      $this->assertResourceErrorResponse(404, 'No route found for "POST ' . str_replace($this->baseUrl, '', $this->getPostUrl()->setAbsolute()->toString()) . '"', $response);
   +    }
   

   We need a documentation for these things

3. +++ b/core/modules/user/user.routing.yml
   @@ -117,6 +117,7 @@ user.page:
        _controller: '\Drupal\user\Controller\UserController::userPage'
        _title: 'My account'
      requirements:
   +    _method: 'GET'
        _user_is_logged_in: 'TRUE'
    
   

   I think this solution makes sense. In general I'm wondering whether it might be a problem on way more routes conceptually.