ControllerResolver::getArguments() is removed in Symfony 4 and ControllerResolver no longer implements ArgumentResolverInterface

Nit. Looks like some whitespace issues.

This appears to be only called by our test coverage, so another option would be to remove the test coverage and treat the inherited method as 'dead code'.

Is this only legacy tests that are calling it? If so they could be skipped if Symfony > 3.

Updated indent of patch and removed a use of ArgumentResolverInterface::class which wasn't used in sprintf.

Excuse me, I've add the wrong interdiff. Here is the correct one.

Done some more thinking about this. I think we need to add a deprecation to our new getArguments() because ideally we don't want people to use it. But if we want people to be able to use Symfony 4 on Drupal 8 then we need to provide it. For me though this pseudo-backporting of Symfony code is one of the reasons that I'm suspicious that making Drupal 8 core AND contrib AND custom be able to run with both Symfony 3 and 4 is likely to be complex and quite involving. That doesn't mean that prepping Drupal 8 to support Symfony 4 is wrong as that will result in less work but this is different and feels like a dangerous precedent.

I don't think we should try to support Symfony APIs that core doesn't use/test that contrib might happen to rely on or anything like that though, since that's a promise we may well not be able to keep.

That's going to be hard to discover though. We're far from 100% test coverage and we've changed stuff in core for Symfony 3 -> 4 deprecations already. For me adding this method to our code is the start of us providing a facade to Symfony 4.

The other idea of detecting Symfony 3 maybe could be done with an explicit assertion on whether \Symfony\Component\HttpKernel\Controller\ControllerResolver::getArguments() exists.

I like #12. The changes to @covers seem great to me because we really ought to be only covering our own code and we don't implement getArguments().

1. +++ b/core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.php
   @@ -75,6 +75,10 @@ protected function setUp() {
      public function testGetArguments() {
   +
   +    if (!in_array('Symfony\Component\HttpKernel\Controller\ArgumentResolverInterface', class_implements('Symfony\Component\HttpKernel\Controller\ControllerResolver'))) {
   

   Nit: extraneous \n.

2. +++ b/core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.php
   @@ -240,12 +246,14 @@ public function testGetArgumentsWithRouteMatchAndRequest() {
   +    if (!in_array('Symfony\Component\HttpKernel\Controller\ArgumentResolverInterface', class_implements('Symfony\Component\HttpKernel\Controller\ControllerResolver'))) {
   

   Why not use ::class?

3. +++ b/core/tests/Drupal/Tests/Core/Controller/FormControllerTest.php
   @@ -20,6 +20,10 @@ class FormControllerTest extends UnitTestCase {
   +    if (!in_array('Symfony\Component\HttpKernel\Controller\ArgumentResolverInterface',                                                                       class_implements('Symfony\Component\HttpKernel\Controller\ControllerResolver'))) {
   

   This looks like invalid PHP. Yet … it did pass tests. WTF? 😱

This looks like invalid PHP. Yet … it did pass tests. WTF? 😱

Apparently that's dreditor screwing this up. First time that ever happened to me.

Fair! LGTM then :) Trivial test-only changes, really.

#17.1 can be fixed on commit (or not at all, it's super nitpicky).

I don't think asset library system is the right component though.

All the tests being changed here are already legacy tests so this looks great to me.

Committed 8b6fd65 and pushed to 8.7.x. Thanks!

Fixed the unnecessary new line on commit.

Also fixed

PHPCS: core/tests/Drupal/Tests/Core/Controller/ControllerResolverTest.php passed

FILE: ...l/core/tests/Drupal/Tests/Core/Controller/FormControllerTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 23 | ERROR | [x] Expected 1 space after comma in function call; 71
    |       |     found
 23 | ERROR | [x] Expected one space after the comma, 71 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 46ms; Memory: 4Mb

PHPCS: core/tests/Drupal/Tests/Core/Menu/LocalActionManagerTest.php passed

FILE: ...rupal/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 254 | ERROR | [x] Expected 1 space after comma in function call; 71
     |       |     found
 254 | ERROR | [x] Expected one space after the comma, 71 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

I guess these were the cause of the dreditor weirdness :)