Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead

This is what we want to remove (this will fail, because I'm not using the new API yet).

This is what the fix should look like, by using the new API.

Unfortunately (but understandably and reasonably!) we also need to add @group legacy … everywhere.

When I run this locally, I get the following error when using the trait that #2935755: Add a trait to allow dynamic setting of expected deprecations introduced:

/usr/local/bin/php /private/var/folders/f6/g_q2g6w545z_q3h4cx7kv2qm0000gn/T/ide-phpunit.php --configuration /Users/wim.leers/Work/d8/core/phpunit.xml --filter "/::testGet( .*)?$/" Drupal\Tests\rest\Functional\EntityResource\Node\NodeJsonBasicAuthTest /Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeJsonBasicAuthTest.php
Testing started at 11:12 ...
#!/usr/bin/env php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\rest\Functional\EntityResource\Node\NodeJsonBasicAuthTest
F

Time: 8.13 seconds, Memory: 6.00MB

There was 1 failure:

1) Drupal\Tests\rest\Functional\EntityResource\Node\NodeJsonBasicAuthTest::testGet
Can not set an expected deprecation message because the Symfony\Bridge\PhpUnit\SymfonyTestsListener is not registered as a PHPUnit test listener.

/Users/wim.leers/Work/d8/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php:63
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:714

FAILURES!
Tests: 1, Assertions: 141, Failures: 1.

#9: it does look like that.

You can see in the failures above that my local environment results in the same error as on DrupalCI.

(I did forgot to add a few @group legacy lines, fixing that here.)

We (@alexpott, @Berdir and I) discussed this on IRC:

15:13:31 <alexpott> WimLeers: ping
15:22:01 <WimLeers> alexpott: pong
15:22:17 <alexpott> WimLeers: I'm just looking at https://www.drupal.org/project/drupal/issues/2936704
15:22:50 <WimLeers> alexpott: yeah, unexpected sadness there :(
15:22:56 <alexpott> WimLeers: is every test a legacy test - ie. does every method cause deprecations to be thrown?
15:23:07 <WimLeers> alexpott: not every method, no
15:23:12 <WimLeers> alexpott: only `testGet()`
15:23:28 <alexpott> WimLeers: so only that method needs the @group
15:23:49 <WimLeers> alexpott: omg I didn't even know you could set `@group` on _methods_!?
15:23:54 <alexpott> WimLeers: also is it truly legacy? i.e. should we remove it in D9
15:24:18 <alexpott> WimLeers: and is there are way to test the non-legacy path?
15:27:09 <WimLeers> alexpott: `testGet()` is testing much more than only legacy. But it's testing _some_ legacy bits. Ideally, we'd add a new `testDeprecatedRoute()`-like method. But that'd further increase test run times …
15:27:26 <WimLeers> alexpott: I'd be fine with that, it'd sure make things cleaner
15:27:37 <WimLeers> alexpott: but there's a definite infra cost to that
15:27:39 <alexpott> WimLeers: yeah but that we'd be able to automated the removal of the legacy test
15:27:50 <WimLeers> alexpott: can you rephrase that?
15:28:40 <alexpott> WimLeers: when we open d9 and remove all the legacy code - tests or test meothds maked with @legacy should be removed too. If a test mixes legacy testing and non-legacy testing we can't do that and we have to apply brain power.
15:29:26 <berdir> WimLeers: about infra costs, the question is if we really need to test some deprecated bits on every single entity type we have. I'd assume it's always the same deprecated logic? We could have a single test method for a test entity type that tests those bits
15:30:07 <WimLeers> alexpott: oh, NICE
15:30:17 <WimLeers> alexpott: I never knew that, that makes me happy
15:30:19 <berdir> WimLeers: if we remove 2 or 3 http calls from multiple hundred tests and add one new test method then that might actually be a win in terms of test execution time
15:31:05 <berdir> e0ipso: sure, take your time, no pressure. Just wanted to make sure it's on your radar :)
15:31:34 <alexpott> WimLeers: yeah the whole deprecation / legacy stuff done right should hopefully mean we don't end up with things like we had with @todos from d6 in d8
15:33:11 <WimLeers> berdir: setup time far outweights number of HTTP requests. Proof: testDelete() does half a dozen HTTP requests in7.64 s, testGet() does a multiple of that in 7.69 s, testPatch() does many dozens in 7.4 s.
15:33:37 <WimLeers> alexpott: I have a fun issue for you related to this that will make you both smile and weep then: https://www.drupal.org/node/2893804
15:33:39 <drupalbot> https://www.drupal.org/node/2893804 => [PP-1] Remove rest.module BC layers #2893804: Remove rest.module BC layers => 22 comments, 1 IRC mention
15:34:26 <alexpott> WimLeers: i've being trying to get Symfony 3.4 working with D8... tears and laughter already today :)
15:34:30 <WimLeers> :D
15:35:03 <WimLeers> alexpott: see berdir's message from 14:30:19 and my reply to that at :33:11. Thoughts?
15:36:31 <berdir> WimLeers: yeah, certainly. but even if it's not actually faster, having one additional method is a no-brainer compared to 400 additional test methods :)
15:36:57 <alexpott> WimLeers: damn... isolated tests and \Drupal\Tests\Traits\ExpectDeprecationTrait::expectDeprecation() ain't gonna work.
15:37:49 <WimLeers> berdir: Oh you mean one new test method that is NOT in in the base class.
15:37:57 <berdir> WimLeers: exactly
15:38:15 <WimLeers> alexpott: :(
15:38:26 <berdir> WimLeers: because I think that's a general question that we should ask ourself.. do we really need to test every single one of those special cases for every single entity type :)
15:38:40 <WimLeers> berdir: yeah fair
15:38:42 <alexpott> WimLeers: isolated tests don't have the listeners attached in the inside process. Sad. so somehow we have to communicate the expected deprecations up.
15:39:22 <WimLeers> berdir: but to also be fair … that's what we said about REST/normalizers/etc too, and turns out there's lots and lots and lots of entity type-specific, format-specific, auth provider-specific, routing edge case-specific bugs
15:39:34 <WimLeers> berdir: which is how we ended up with this gazillion integration tests
15:39:43 <berdir> WimLeers: I also commented again on https://www.drupal.org/project/drupal/issues/2919373#comment-12421026 for when you're done with this discussion :)
15:39:45 <WimLeers> berdir: I agree we don't need that in THIS case though
15:39:46 <alexpott> WimLeers: i think we need a file.
15:39:54 <WimLeers> alexpott: a file?
15:40:14 <alexpott> WimLeers: yeah the inner process writes a file and the outer reads it.
15:40:29 <alexpott> WimLeers: and sets the expected deprecations
15:40:46 <WimLeers> alexpott: My response to that is "ehhhhhhhhh"
15:41:30 <WimLeers> alexpott: You have a more complete picture of all the deprecation stuff and test infra.
15:41:43 <WimLeers> alexpott: but I'd think this is not high-priority enough for you to look at _today_, the day the alpha is tagged
15:41:46 <WimLeers> alexpott: hf!
15:43:00 <alexpott> WimLeers: yeah but I have Symfony PR out there that has the same problem
15:46:16 <WimLeers> alexpott: I see

So, in this patch:

1. removing the test logic from EntityResourceTestBase::testGet()
2. adding EntityTestResourceTestBase::testFormatSpecificGetBcRoute() for testing the deprecation
3. adding EntityTestResourceTestBase::testNoFormatSpecificGetBcRouteForOtherFormats() for testing that only the appropriate BC routes are created
4. both points 2 and 3 run only for the entity_test entity type, running it for all entity types is overkill. They do run for all formats though!
5. both methods are tagged @group legacy

Obviously you're right — I got that wrong in my copy paste haste. Didn't help that I can't run the test, of course!

Thanks for #2936802: expectDeprecation() doesn't work in isolated tests!

This is now blocked on #2936802: expectDeprecation() doesn't work in isolated tests.

❤️ Thanks @amateescu!

Is there any reason to have these as separate test methods?

Because Alex Pott requested that. Quoting relevant bits from chat log in #12:

15:27:09 <WimLeers> alexpott: `testGet()` is testing much more than only legacy. But it's testing _some_ legacy bits. Ideally, we'd add a new `testDeprecatedRoute()`-like method. But that'd further increase test run times …
15:27:26 <WimLeers> alexpott: I'd be fine with that, it'd sure make things cleaner
15:27:37 <WimLeers> alexpott: but there's a definite infra cost to that
15:27:39 <alexpott> WimLeers: yeah but that we'd be able to automated the removal of the legacy test
15:27:50 <WimLeers> alexpott: can you rephrase that?
15:28:40 <alexpott> WimLeers: when we open d9 and remove all the legacy code - tests or test meothds maked with @legacy should be removed too. If a test mixes legacy testing and non-legacy testing we can't do that and we have to apply brain power.

Sounds good! To not have 3 identical implementations of both test methods in those 3 classes, I moved both test methods into a trait that all 3 classes use.

I see I didn't respond to the second option suggested in #25:

Or maybe a new subclass of EntityTestResourceTestBase and change the test methods to not look at static::$format but loop through all 3 formats in one place?

That would result in non-isolated tests and therefore potential side effects interfering, because we're testing something that depends on configuration.

That's why I implemented the first suggestion of #25.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/FormatSpecificGetBcRouteTestTrait.php
@@ -6,12 +6,15 @@
- * @group internal
...
+ * @internal

😱😅

STUPID STUPID STUPID Wim 😆

Thanks, Andrei!