Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent

This is basically all that's needed in this issue.

Let's be more clear in ResourceBase on why DELETE doesn't need _format nor _content_type_format route requirements. Explicit documentation helps prevent questions later.

I didn't notice that I also needed to update DbLogResourceTest. Easy fix. That fixes one of the failures.

All remaining failures are the same. They're all about testPost() failing at EntityResourceTestBase.php:633:

Failed asserting that 406 is identical to 415.

(And because #2832859: Write EntityResourceTestBase subclasses for: MenuLinkContent just got committed, I think the number of failures will actually be higher in #7 than in #3, despite fixing one failure).

Why is this happening?

1. Because the POST route only had _content_type_format before, now it has _format.
2. This causes \Drupal\Core\Routing\RequestFormatRouteFilter::applies() to return TRUE instead of FALSE.
3. The compiled route has a route option called _route_filters. It used to be ['method_filter', 'content_type_header_matcher'] before this patch. But point 2 causes it to be ['request_format_route_filter', 'method_filter', 'content_type_header_matcher'] after this patch.
4. And then we get to the actual bug: #2368769: All route enhancers are run on every request introduced \Drupal\Core\Routing\LazyRouteFilter and effectively made every route filter a lazy one. But… it did so without respecting the priority of route_filter-tagged services! Why was this not noticed? Because REST had almost no test coverage. And neither does the routing system for WSCCI functionality, i.e. REST functionality. And so the handful of route filters that we have just ran in a random order (well, somewhat deterministic, but definitely not respecting the service priority).

On top of that, because nothing in the entirety of Drupal 8: not the REST module, not anything else in core, not even the routing system test coverage… had both a _format and a _content_type_format route requirement. And hence… we didn't even notice that even the existing route_filter service priorities were incorrect. method_filter had priority 1, all others (request_format_route_filter and content_type_header_matcher) had no priority, i.e. priority 0.
But content_type_header_matcher must run before request_format_route_filter! Because when a route has a _content_type_format restriction, that means it accepts/needs a request body. Without the request body, no sensible response. Missing a Content-Type request header causes a 415. Missing a ?_format (or Accept request header in an ideal world) causes a 406.

The tests are failing because a 415 is expected, but a 406 is received. In other words: exactly the problem I just explained.

So this patch then:

1. causes LazyRouteFilter to respect the priorities for route_filter-tagged services. It looks at \Drupal\Core\Routing\Router for guidance — it uses the exact same approach.
2. also fixes the priority of the content_type_header_matcher: it must run before the request_format_route_filter

Yes, this arguably belongs in a separate issue. We can debate that later. I first want to get this to green. To prove that this solves the problems uncovered here.

#2368769: All route enhancers are run on every request also didn't make ContentTypeHeaderMatcher actually lazy. Let's do that.

This fixes all fails.

In \Drupal\rest\Tests\ResourceTest::testSerializationClassIsOptional() and \Drupal\user\Tests\RestRegisterUserTest::registerRequest(), the ?_format= query string was missing for POST requests.

And \Drupal\rest\Tests\Update\ResourceGranularityUpdateTest failed because \Drupal\rest\Routing\ResourceRoutes::getRoutesForResourceConfig() now is updating _format correctly for the first time. In doing so, it is omitting the hal_json format… because the hal module was not actually enabled in this test! So the fix was simple, change:

$extensions = unserialize($extensions);
$extensions['module']['rest'] = 0;
$extensions['module']['serialization'] = 0;
$connection->update('config')
  ->fields([
    'data' => serialize($extensions),
  ])

to:

$extensions = unserialize($extensions);
$extensions['module']['hal'] = 0;
$extensions['module']['rest'] = 0;
$extensions['module']['serialization'] = 0;
$connection->update('config')
  ->fields([
    'data' => serialize($extensions),
  ])

Aha. I didn't realize that when I first read your comment. Sorry! Thanks for taking the time to point it out so clearly :)

We can implement it without using service_collector in this patch, to avoid making it non-lazy. Did that in this reroll.

This blocks #2737751: Refactor REST routing to address its brittleness.

I forgot --binary. This is the patch that the one in #16 should have been.

1. Agreed! Done. (This was done in #9, detailed explanation there.)
2. Done.
3. Hah, nice simplification, done :)

RE: REST GET routes going from one-per-format-per-resource (N) to one-per-resource (1): change record created: https://www.drupal.org/node/2865645.

#24:

1. Okay, let's postpone this on #2472337: Provide a lazy alternative to service collectors which just detects service IDs then. (I didn't mean to annoy/offend, I just wanted to see some progress. It takes forever to get anything REST-related committed, which is why 1/3rd of the core RTBC queue is currently REST-related…)
2. Excellent point. Let's fix that when this is unblocked again.

Updated #2853460: Simplify RequestHandler: make it no longer ContainerAware, split up ::handle() to indicate that it's now blocked on a chain of two issues. And updated #2737751: Refactor REST routing to address its brittleness to indicate it's now blocked on a chain of 3 issues.

#2472337: Provide a lazy alternative to service collectors which just detects service IDs just landed, so this is now unblocked!

Rerolling…

This conflicted with #2826407: PATCH + POST allowed format validation happens in RequestHandler::handle(), rather than in the routing system.

1. This reroll only resolves the conflict; it does not address #24.
2. I also still have to look into potential changes to the approach in this patch, because REST has improved quite a bit since March (2 months ago!), which is when this patch was last updated.

error: cannot apply binary patch to 'core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php' without full index line

git is a git with complex files.

Rerolled with --binary, without any other changes.

Addressed #29.2: I saw ways to further clean this up.

#2826407: PATCH + POST allowed format validation happens in RequestHandler::handle(), rather than in the routing system moved the setting of the _content_type_format route requirement from ResourceBase::routes() to ResourceRoutes::getRoutesForResourceConfig. But this patch was adding the setting of the _format route requirement to ResourceBase::routes(). Ideally both would be set in the same place, right? Because both suffer from the same problem: we can't set them in ResourceBase::routes() (or ResourceInterface::routes() in general) because REST resource plugins do not have access to the REST resource configuration…

So if we accept that premise, and we then choose to remove the setting of _format from ResourceBase::routes(), then there's nothing left to do for the PATCH, GET, HEAD and DELETE methods. The only logic that remains, is that for POST, to specify a different path than the canonical one.

Which means we can delete that entire confusing switch statement, and make it super simple. Then it becomes very clear that ResourceBase::routes() really actually is returning base routes by default: the _format and _content_type_format have to be specified later, when we can read the actual configured formats for this resource from its config entity!

This also means that the changes this patch was making to ResourceRoutes:::getRoutesforResourceConfig() actually can
be simplified to match that of #2826407: PATCH + POST allowed format validation happens in RequestHandler::handle(), rather than in the routing system. And we can even slightly simplify the logic that #2826407 added there, and make both the _content_type_format and the _format cases consistent.

Addressed #24.1, by using the new functionality introduced by #2472337: Provide a lazy alternative to service collectors which just detects service IDs. I followed the best practices/core committer requirements that were established in that issue, which includes not using ContainerAware. I also tried to minimize changes in LazyRouteFilter, but felt obligated to correct the most misleading pieces of documentation present in LazyRouteFilter.

Note: it's probably easier to not interview the interdiff, but review the overall patch, and look specifically at the LazyRouteFilter-related changes.

IS updated, CR (https://www.drupal.org/node/2598944) updated.

#24.2: you're right. But in order to achieve that, we'd have to ensure that \Drupal\Core\Routing\LazyRouteFilter::filter() actually respected the the route filters specified for each specific route. And that's not how \Symfony\Cmf\Component\Routing\NestedMatcher\RouteFilterInterface (filter()) was designed: it checks every route in the collection, regardless. Drupal layered \Drupal\Core\Routing\RouteFilterInterface (applies()) on top of that, solely for the route compilation phase.

i.e. applies() determines which filters need to be applied to a collection of initial route matches (based on URL) to filter() the route collection down to a smaller number (based on method, format, etc).

What you're saying means that every route filter implementation must ensure that every filter() implementation must also call applies() for every route it iterates over in its collection. Which means requiring pure Symfony API implementations (just filter()) to be aware of the Drupal API addition (applies()) and conditionally change their logic.

So I think it's safer to keep this as it is today. We could open a separate issue for that though.

The change you quoted is still valid: it ensures that we only run the ContentTypeHeaderMatcher route filter when there's actually at least one route in the route collection that needs it!

Whew, now blocked on review!

I just proposed a change to make route filters and enhancers much better for themselves

Commented there. +1 :) But I don't think you are saying that issue should block this one, right?

This would be indeed possible, but it feels a bit like an overoptimisation.

Fine, reverted.

(I'm confused by this though: ContentTypeHeaderMatcher is only very rarely necessary, yet HEAD (and this revert) causes it to run always. So then what's the point of it all? Of course, if I understand #2883680: Force all route filters and route enhancers to be non-lazy correctly, it'll remove this altogether, so then making this change now indeed is pointless.)

This was first blocked on #2472337: Provide a lazy alternative to service collectors which just detects service IDs for almost 3 months. The day after that landed, I got this patch going again (#28 through #36).

But now it's being suggested in #37 that this gets blocked on #2883680: Force all route filters and route enhancers to be non-lazy. I think #2883680 is a worthwhile simplification. But despite me having worked on that for half a day today to get it to green, it's still not green. And for it to get to RTBC, performance (profiling) will have to show positive results (i.e. faster or same speed).

I'm fine with delaying this on #2883680: Force all route filters and route enhancers to be non-lazy for a few weeks, but I don't want this to be blocked again for multiple months. That'd mean 3 REST issues are again blocked for months (this issue, plus #2853460: Simplify RequestHandler: make it no longer ContainerAware, split up ::handle() which is blocked on this, plus #2737751: Refactor REST routing to address its brittleness which is blocked on that). If this were introducing nasty work-arounds, then I'd agree with you that we should hard-block this. on #2883680. But this issue is merely fixing the existing infrastructure, #2883680 is completely refactoring the existing infrastructure.

Postponing for now.

#2883680: Force all route filters and route enhancers to be non-lazy (finally — after almost 4 months…) landed, which means this is now fully unblocked!

1. index 796ece3..a59190c 100644
   --- a/core/core.services.yml
   
   index 8a22bc5..719df12 100644
   --- a/core/lib/Drupal/Core/CoreServiceProvider.php
   
   index d774779..0000000
   --- a/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterLazyRouteFilters.php
   
   index 356fe3e..5299604 100644
   --- a/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
   
   index 10f3d51..f6672ff 100644
   --- a/core/lib/Drupal/Core/Routing/LazyRouteFilter.php
   

   All these changes are obsolete now that #2883680: Force all route filters and route enhancers to be non-lazy is in! :)

2. --- a/core/modules/rest/src/Tests/ResourceTest.php
   +++ b/core/modules/rest/src/Tests/ResourceTest.php
   

   The changes here are obsolete because #2775553: Convert web tests to browser tests for REST module refactored this test a few months ago.

So many obsolete changes, we can drop all their hunks, which means this patch just became a lot smaller!

I omitted too many hunks in #41:

+++ b/core/core.services.yml
@@ -938,15 +939,21 @@ services:
   request_format_route_filter:
     class: Drupal\Core\Routing\RequestFormatRouteFilter
     tags:
+      # The request format route filter must run last.
       - { name: route_filter }
   method_filter:
     class: Drupal\Core\Routing\MethodFilter
     tags:
-      - { name: route_filter, priority: 1 }
+      # The HTTP method route filter must run first: based on the request method, content type request header-based
+      # route filtering (content_type_header_matcher) may or may not be necessary.
+      - { name: route_filter, priority: 10 }
   content_type_header_matcher:
     class: Drupal\Core\Routing\ContentTypeHeaderMatcher
     tags:
-      - { name: route_filter }
+      # The content type request header router filter must run before the request format route filter
+      # (request_format_route_filter), because without a valid request body (for HTTP methods that need it, such as POST
+      # and PATCH), no successful response is possible.
+      - { name: route_filter, priority: 5 }

These fixes the the current priorities (plus comments to explain/justify them) are still necessary.

IS updated. IS now also explains why #43 is making the patch pass.

Thanks for the review! 👏

1. ✅ Done.
2. ✅ AFAICT yes. Thanks, fixed!
3. ❓ Keeping them around for BC would completely defeat the purpose of this issue, which is to make REST routing actually maintainable. I think the number of people overriding/altering REST routes can be counted on two hands at most, and the change in code that this patch would require them to make would make their code actually simpler.
4. ✅ Okay, done. (My rationale for doing it this way is that the === POST is the less common case. I always put the common case first. I don't feel have a strong opinion about this though.)

That's fair. If at all possible, we shouldn't break BC. And it's possible in this case. Although doing this does mean

We could probably provide some routes which aren't involved in actual routing, simply for BC reasons?

Symfony's routing system has no concept of "route aliases", so we can't do this. There are only two alternatives:

1. use Symfony subrequests
2. provide duplicate route definitions

The latter is the simplest approach, and is also the strategy we used last time we moved routes around (in #2753681: Move CSRF header token out of REST module so that user module can use it, as well as any contrib module).

<front> and <current> don't use the (Symfony) routing system. They use Drupal's special sauce on top: path/route processors. \Drupal\Core\PathProcessor\PathProcessorFront + \Drupal\Core\RouteProcessor\RouteProcessorCurrent.

But <front> and <current> also exist in the router. They just have very thin route definitions, because the bulk of the logic lives in path/route processors. Why is that better/preferable to what #48 does?

#53 is an excellent point. I hadn't looked at it that way yet. I'll try to do that instead.

I'm glad @dawehner made that remark and insisted on it — \Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface is indeed the perfect fit! Thanks, Daniel!

It seems to work nicely. It is a bit more code, but indeed helps ensure it's a BC layer with the smallest surface possible.

Testbot is working again. Retesting, back to NR.

Sigh, forgot git diff --binary.

#60:

1. A. I can never remember. Went with the 3 spaces you asked for.
   B. Done. Note that there is no way to fix this without breaking BC, which is why this comment exists. I started opening a new issue, but then I realized that this is pretty much how it was designed to work. I think that this cannot be completely resolved, not without rearchitecting more of the REST module. Such rearchitecting will only make sense after all of #2905563: REST: top priorities for Drupal 8.5.x is done, because that that point we'll have a much clearer perspective/overview of what works well and what doesn't. So, simply removed those @todos.
2. Yep! Thanks :)
3. Done.

#61:

1. Interesting, I was thinking along similar lines. This is why I went with bc_route as the route option, because it'd allow all BC routes in D8 to do something like that: so you could easily remove all BC routes.
2. Oh I guess you're asking for config specific to this particular route. I'm not sure what the value is of having a BC flag in config for every BC route of every module. Who's realistically going to use those flags? I guess the advantage would be that any site that is new to Drupal 8.5 would have that BC layer turned off by default, so that only older D8 sites get these BC routes generated? That makes sense. Done.

   This was a lot more code to do than I thought/hoped… but such is the nature of BC layers.

I forgot to

1. account for the case where RestServiceProvider runs before rest_update_8501() has run
2. update UserRegistrationResource

This should reduce the number of failures. (The patch is now twice as large as #62… 😨)

The remaining failures look like this: Exception: TypeError: Argument 1 passed to Drupal\rest\RouteProcessor\RestResourceGetRouteProcessorBC::__construct() must be of the type array, object given — and I have NFI why that's happening…

This is identical to @dawehner's patch in #67, just rebased, so it applies again.

So apparently @dawehner's interdiff indeed solves the problems we saw in #65 … but it causes failures in UpdatePathTestBaseTest and UpdatePathTestBaseFilledTest :(

Thanks for debugging! Tricky, isn't it…

But other than the fact that it's currently not passing tests, do you prefer the approach in #63 (BC routes exist only if config enables them) and later to the one in #62 (BC routes exist always)? You requested it, but I want to make sure you like the outcome, assuming we can get it to green. If you don't like it, then we also don't need to debug it further.

#2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage conflicts with this in two places, in trivial ways. Rebased.

Also fixed the coding standards violations.

Assigning to @dawehner for feedback on #73, regarding the new direction he requested.

error: cannot apply binary patch to 'core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php' without full index line

😭

Rerolled with --binary.

Looks like reverting #67's interdiff makes tests pass :)

Forgot --binary 🤦‍♂️ Rerolled.

For contrib module reasons we need to enable those routes actually by default, otherwise new installed modules would fail as well.

For contrib module reasons we need to enable those routes actually by default, otherwise new installed modules would fail as well.

If that's the case, then do we even want/need this flag in config? Who is ever going to find out about this BC flag, and is going to manually disable it?
Is your intent to first enable the BC routes always (for both existing and new sites), and then in a future minor to not install the BC routes on new sites, because contrib modules will have had time to migrate?

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -53,18 +53,18 @@ class EntityResource extends ResourceBase implements DependentPluginInterface {
   -   * The config factory.
   +   * The link relation type manager used to create HTTP header links.
       *
   -   * @var \Drupal\Core\Config\ConfigFactoryInterface
   +   * @var \Drupal\Component\Plugin\PluginManagerInterface
       */
   -  protected $configFactory;
   +  protected $linkRelationTypeManager;
    
      /**
   -   * The link relation type manager used to create HTTP header links.
   +   * The config factory.
       *
   -   * @var \Drupal\Component\Plugin\PluginManagerInterface
   +   * @var \Drupal\Core\Config\ConfigFactoryInterface
       */
   -  protected $linkRelationTypeManager;
   +  protected $configFactory;
   

   Unnecessary/out of scope change: these are just being switched places. Reverted.

2. +++ b/core/modules/rest/src/RouteProcessor/RestResourceGetRouteProcessorBC.php
   @@ -0,0 +1,79 @@
   +  public function processOutbound($route_name, Route $route, array &$parameters, BubbleableMetadata $bubbleable_metadata = NULL) {
   +    $route_name_parts = explode('.', $route_name);
   +    // BC: the REST module originally created per-format GET routes, instead
   +    // of a single route. To minimize the surface of this BC layer, this uses
   +    // route definitions that are as empty as possible, plus an outbound route
   +    // processor.
   +    // @see \Drupal\rest\Plugin\ResourceBase::routes()
   +    if ($route_name_parts[0] === 'rest' && $route_name_parts[count($route_name_parts) - 2] === 'GET' && in_array($route_name_parts[count($route_name_parts) - 1], $this->serializerFormats, TRUE)) {
   +      array_pop($route_name_parts);
   +      $redirected_route_name = implode('.', $route_name_parts);
   +      static::overwriteRoute($route, $this->routeProvider->getRouteByName($redirected_route_name));
   +    }
   +  }
   

   This needs a deprecation error to be thrown. Expect lots of failures due to that.

Ugh … so #79/#81 fixed the 2 failing tests … but causes all update path tests to fail like this:

Drupal\Tests\comment\Functional\Update\CommentUpdateTest::testCommentUpdate8101
Exception: TypeError: Argument 1 passed to Drupal\rest\RouteProcessor\RestResourceGetRouteProcessorBC::__construct() must be of the type array, object given
Drupal\rest\RouteProcessor\RestResourceGetRouteProcessorBC->__construct()() (Line: 37)

because the serializer.formats is apparently an object instead of an array. Just like I wrote in #65.

Sigh. Undoing #79/#81, aka repeating #67.

That is a good point. Well I was hoping people can disable it and run with it, if it works.

So, you agree we can remove the flag? The deprecation notice is enough to encourage users to update I think.

I think the patch looks fine right now, but I think it lacks one essential feature: Actually throw a deprecation error when someone uses the deprecated routes. Much like we want to for example add support for deprecated permissions we want to do essentially the same for routes as well. I'm happy to work on that. Overall I'm not 100% sure whether this should block this issue or not, but I think the general feature would be needed.

Sounds good! 👍

Alright, on it then. That will also remove the need for the container rebuild, which means the question in #78.2 for Alex Pott is no longer necessary.

That allowed for a massive simplification: from 18 files changed, 245 insertions(+), 60 deletions(-) to 8 files changed, 139 insertions(+), 42 deletions(-).

The set of changes now looks much more coherent, and hence much easier to review :)

IDK why, but DeprecationListener doesn't seem to work for me (not in PHPStorm, not using phpunit directly, and yes, with updated phpunit.xml and up-to-date vendor). Is this because class EntityResourceTestBase extends class ResourceTestBase extends BrowserTestBase?

(I also tried using @deprecatedException + @group legacy, but that also failed. That'd be a nightmare here though, because the exception message is different for every subclass. Trying that is also how I found #2928645: \Drupal\Tests\migrate\Kernel\Plugin\MigrationDirectoryTest uses @expectedDeprecationMessage.)

Things surprisingly work well, when you are doing them right.

😭😇😱

:D

I'm an idiot. So glad you caught that. I could've debugged that for a few more hours before finding that :D

Hm, that also didn't work apparently? :( Did I get something else wrong?

First: rebase. Conflicts with #2927758: Update DbLogResourceTest to use the ResourceTestBase base class instead of the deprecated RESTTestBase (made the patch simpler) and #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes. Easy fixes.

@benjy: Thanks for testing this!

Afterwards I have the XML route enabled even though

Aha!

The reason is that we generate BC routes for all formats, even formats that aren't enabled. That's easy enough to fix :)

Let's start by adding a failing test that detects the regression reported in #101.

And the fix!

Not sure how this is meant to work, because it's a new route it loses the "methods" and therefore never gets a route in ResourceRoutes.php.

It works fine actually, because:

if (($methods && ($method = $methods[0]) && $supported_formats = $rest_resource_config->getFormats($method)) || $route->hasOption('bc_route')) {

That || $route->hasOption('bc_route' is what makes it work. It's intentional that the route definition of this BC route is as empty as possible, because \Drupal\rest\RouteProcessor\RestResourceGetRouteProcessorBC takes care of automatically filling it with the successor route's definition.

Note that #104 is failing not due to the changes I made in those last few patches, it already was failing, due to the BC @trigger_error() stuff.

So: are you sure it's still broken for you?

Added such a comment.

Woah, #110 suddenly passed, without the 413 fails … because something must've changed in the handling of deprecations. Well, YAY, that means we can finally continue here :)

#111: I understand that the first argument to the Route constructor being the empty string looks confusing, but otherwise we still end up putting metadata in those routes, which we specifically don't want to.

I don't know what you're asking for when you say Could we have a new version of the route object to represent deprecated routes? — could you explain that a bit more? I also don't understand what you're getting at when you say Not sure how they are stored in the database, would work if they were serialized?.

#113: AHHHHH! I love that! 😍

Done.

Random infra fail.

WOOOT!

1. This was intentional, it's meant to be just an example, not a must-see reference implementation. I've struggled to choose between what's currently in the patch and @see though. IDK what's best. The sad thing is that @see must be on a line of its own, otherwise I'd do it inline here.
2. 👍 Fixed!

I'm glad we're down to this level of nitpicking :D

Thanks for all your detailed feedback so far, @benjy, much appreciated! 👌

This is very important for maintainability, and the size of the patch reflects the complexity. Therefore, bumping to major.

#2916740: Add generic entity actions conflicted with this, in DeprecationListenerTrait. Rebased.

As of #1927648-490: Allow creation of file entities from binary data via REST requests, this is now also a hard blocker for that issue!

Are you kidding me? :(

The 'rest.entity.entity_test.GET.json' route is deprecated since version 8.5.x and will be removed in 9.0.0. Use the 'rest.entity.entity_test.GET' route instead: 1x

My bet is that #2934336: Update symfony/phpunit-bridge to the latest released version caused this :(

17:59:20 <alexpott> @WimLeers: The upgrade now make deprecation visible again
17:59:47 <alexpott> @WimLeers: your tests are triggering a deprecation "The 'rest.entity.entity_test.GET.json' route is deprecated since version 8.5.x and will be removed in 9.0.0. Use the 'rest.entity.entity_test.GET' route instead"
18:00:08 <alexpott> WimLeers: lol I've been slacked adding @ in front of things
18:03:47 <WimLeers> alexpott: but I'm also updating the DeprecationListenerTrait …
18:03:56 <WimLeers> alexpott: neither dawehner nor I were able to figure this out
18:04:08 <WimLeers> alexpott: I'm hoping you see what obvious mistake we made :D
18:05:14 <alexpott> WimLeers: I'm not sure about adding all that stuff to getSkippedDeprecations - we really shouldn't be adding to that.
18:06:57 <alexpott> WimLeers: loooking
18:09:19 <WimLeers> alexpott: thanks, much appreciate it
18:13:35 <alexpott> WimLeers: you're missing a fullstop
18:15:13 <alexpott> WimLeers: for reasons I don't quite know Symfony strips the final fullstop off the message
18:15:42 <alexpott> WimLeers: but "The 'rest.entity.entity_test.GET.json' route is deprecated since version 8.5.x and will be removed in 9.0.0. Use the 'rest.entity.entity_test.GET' route instead.", works for me
18:16:22 <alexpott> WimLeers: so yay deprecation testing is truly fixed by 2934336
18:16:32 <WimLeers> alexpott: FML
18:16:36 <WimLeers> alexpott++
18:16:39 <WimLeers> alexpott++
18:16:46 — alexpott broke it getting PHP7.2 fixed
18:16:50 <WimLeers> alexpott: So that's why dawehner and I weren't able to spot it
18:16:55 <WimLeers> alexpott: because the strings DO match
18:17:02 <WimLeers> alexpott: it's just that Symfony is modifying them…
18:17:06 <WimLeers> gahhhhhh
18:17:12 <alexpott> WimLeers: yeah I think we should submit a PR to Symfony to stop that
18:17:28 <alexpott> WimLeers: I'll do that
18:17:28 <WimLeers> alexpott: well if that code is there, it's probably intentional
18:17:40 <alexpott> WimLeers: it is to make it a sentence

🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️

To improve this in the future, @alexpott created https://github.com/symfony/symfony/pull/25757

And he even created a Drupal port of it, in case Symfony doesn't merge his PR: #2935755: Add a trait to allow dynamic setting of expected deprecations.

#2853460: Simplify RequestHandler: make it no longer ContainerAware, split up ::handle() landed earlier today.

Like that issue, this issue is blocking #1927648: Allow creation of file entities from binary data via REST requests. I said in #125 already that this was a hard blocker, but the latest patch (#1927648-501: Allow creation of file entities from binary data via REST requests) is now truly based on this.

I think we shouldn't because this blocks #1927648: Allow creation of file entities from binary data via REST requests. It'd be trivial to simplify \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() once #2935755: Add a trait to allow dynamic setting of expected deprecations lands. I'd be happy to guarantee that happens.

1. Heh, non-native-speaker mistake. Fixed!
2. No, because we're interested in 3 different subsections: the very first, the last, and the second last. We don't care about the subsections between the first and the second last. (Also, see next point.)
3. Yes! Good call. It does make the code far more verbose though, but for a good reason probably :) We can't, because this works for any REST resource plugin, and the routes generated are of the form rest.PLUGIN_ID.METHOD.FORMAT. So rest.entity.node.GET.json or rest.dblog.GET.json. Note how explode('.', …) would result in 5 parts for the former, and 4 parts for the latter.
4. I like that idea! But AFAICT it sets the wrong expectation. The point of BcRoute is that it's an as-empty-as-possible route definition. X::fromY() implies constructing a X from Y, but that implies that X inherits all metadata/definition data from Y. Which is exactly what we don't want here.
   It would still work, but it'd encourage the wrong usage. Unless … we also override serialize(), to guarantee that it'll always be an empty definition that gets stored in the router.
   Did that, curious to hear what you think.
5. Sure. _{(But this means we can't add more assertions after this. And adding an entirely new test method for this will slow down every REST test. For now this is fine though!)}

I’m at 🏓, can somebody reupload #129 and RTBC? Thanks!

I think we need #137 minus #136.5, not #129

Right, I realized that long after posting #140!

Sorry for the misdirection on the BcRoute factory, you're too efficient - I posed the question and was expecting discussion first :)

No problem at all! I don't mind exploring these things. It took me <15 minutes to write the implementation. Discussing an actual implementation makes the discussion much more concrete, and hence more effective. It was worth exploring!

DO NOT READ THIS INTERDIFF OR PATCH, I messed things up. Don't skip reading comments, just skip the interdiffs/patch on this comment. Redid the interdiffs/patch in #146.

We also need a follow-up filed to remove all usages of the deprecated GET routes from core, and then remove the messages from the allowed deprecation messages list. I'd love to see that fixed as soon as possible just to keep that list trimmed and terrific. But obviously, let's get through the alpha deadline first.

There are no usages! Only in tests:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -693,15 +694,27 @@ public function testGet() {
+    // BC: Format-specific GET routes are deprecated. They are available on both
+    // new and old sites, but trigger deprecation notices.
+    $bc_route = Url::fromRoute('rest.entity.' . static::$entityTypeId . '.GET.' . static::$format, $url->getRouteParameters(), $url->getOptions());
+    $bc_route->setUrlGenerator($this->container->get('url_generator'));
+    $this->assertSame($url->toString(TRUE)->getGeneratedUrl(), $bc_route->toString(TRUE)->getGeneratedUrl());
+    // Verify no format-specific GET BC routes are created for other formats.
+    $other_format = static::$format === 'json' ? 'xml' : 'json';
+    $bc_route_other_format = Url::fromRoute('rest.entity.' . static::$entityTypeId . '.GET.' . $other_format, $url->getRouteParameters(), $url->getOptions());
+    $bc_route_other_format->setUrlGenerator($this->container->get('url_generator'));
+    $this->setExpectedException(RouteNotFoundException::class);
+    $bc_route_other_format->toString(TRUE);

This is the only way the deprecation error is being triggered, and it's the reason we're updating \Drupal\Tests\Listeners\DeprecationListenerTrait. That is why I worked with @alexpott to get #2935755: Add a trait to allow dynamic setting of expected deprecations going. Once that lands, we can change the test coverage to use expectDeprecation($message) instead. Without that, we'd have to have a method that every subclass of EntityResourceTestBase needs to override, to be able to set the appropriate @expectedDeprecation docblock.

IOW: as soon as #2935755: Add a trait to allow dynamic setting of expected deprecations lands, we can simplify DeprecationListenerTrait again!

+++ b/core/modules/dblog/tests/src/Functional/DbLogResourceTest.php
@@ -62,7 +62,7 @@ public function testWatchdog() {
-    $url = Url::fromRoute('rest.dblog.GET.' . static::$format, ['id' => $id, '_format' => static::$format]);
+    $url = Url::fromRoute('rest.dblog.GET', ['id' => $id, '_format' => static::$format]);

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -693,15 +694,27 @@ public function testGet() {
-    $url = Url::fromRoute('rest.entity.' . static::$entityTypeId . '.GET.' . static::$format);
+    $url = Url::fromRoute('rest.entity.' . static::$entityTypeId . '.GET');

+++ b/core/modules/user/src/Tests/RestRegisterUserTest.php
@@ -166,7 +167,7 @@ protected function registerUser($name, $include_password = TRUE) {
-    $this->httpRequest('/user/register', 'POST', $serialized, 'application/hal+json');
+    $this->httpRequest(Url::fromRoute('rest.user_registration.POST', ['_format' => 'hal_json']), 'POST', $serialized, 'application/hal+json');

These are all existing uses, being updated.

I completely messed up #142.

So, #142 was completely wrong. This completely ignores #142. Again with interdiffs from #129 and #137, but this time … correct. 🤞

Thanks @dawehner!

This also unblocked #1927648: #1927648-514: Allow creation of file entities from binary data via REST requests.

Created #2936704: Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead for #150.