Problem/Motivation

#2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent added lots of skipped deprecations to DeprecationListenerTrait::getSkippedDeprecations().

The missing API to deal with this more elegantly (to allow expected deprecations that aren't hardcoded in docblocks: ExpectDeprecationTrait::expectDeprecation()) was added in #2935755: Add a trait to allow dynamic setting of expected deprecations.

Proposed resolution

Remove #2858482's additions to skipped deprecations, instead use #2935755's new API.

Remaining tasks

  1. Patch.
  2. Get green.
  3. Review + RTBC.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

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

Wim Leers’s picture

FileSize
1.82 KB
26.59 KB

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

Wim Leers’s picture

FileSize
269.91 KB
288.18 KB

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

Wim Leers’s picture

Issue summary: View changes

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.

The last submitted patch, 2: 2936704-2.patch, failed testing. View results

The last submitted patch, 3: 2936704-3.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: 2936704-4.patch, failed testing. View results

alexpott’s picture

  <listeners>
    <listener class="\Drupal\Tests\Listeners\DrupalListener">
    </listener>
    <listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener">
    </listener>
  </listeners>

@Wim Leers make sure your core/phpunit.xml looks something like that ^^

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
290.33 KB

#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.)

Status: Needs review » Needs work

The last submitted patch, 10: 2936704-10.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review

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
Wim Leers’s picture

FileSize
4.76 KB
29.51 KB

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

Status: Needs review » Needs work

The last submitted patch, 13: 2936704-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php
@@ -158,4 +162,37 @@ protected function getExpectedUnauthorizedAccessMessage($method) {
+    $this->expectDeprecation(sprintf("The 'rest.entity.entity_test.GET.%s' route is deprecated since version 8.5.x and will be removed in 9.0.0. Use the 'rest.entity.entity_test.%s' route instead.", static::$format, static::$format));

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -141,144 +141,6 @@ public static function getSkippedDeprecations() {
-      "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.",

I'm not sure this is correct. The new route seems to have GET on the end...rest.entity.%s.GET

Also #2936802: expectDeprecation() doesn't work in isolated tests should allow this to work.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.46 KB
29.5 KB

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!

Wim Leers’s picture

Title: Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead » [PP-1] Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead
Status: Needs review » Postponed

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

Title: [PP-1] Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead » Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead
Status: Postponed » Needs work

#2936802: expectDeprecation() doesn't work in isolated tests got in last week so this is no longer postponed.

amateescu’s picture

Status: Needs work » Needs review
FileSize
30.05 KB
674 bytes

#16 needed a reroll and a small CS fix, seems to be working fine locally.

Wim Leers’s picture

❤️ Thanks @amateescu!

amateescu’s picture

Now.. looking at the patch a bit:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php
@@ -158,4 +162,37 @@ protected function getExpectedUnauthorizedAccessMessage($method) {
+  public function testFormatSpecificGetBcRoute() {
...
+  public function testNoFormatSpecificGetBcRouteForOtherFormats() {

Is there any reason to have these as separate test methods? In practice, this means we add (2 * number of rest entity types) browsers tests, which are quite slow.

Berdir’s picture

@amateescu: This tricked my as well, but note that they are actually on Entity*Test*ResoureceTestBase, which means we "only" run those for the entity_test entity type tests.

That said, that's still about 14 different tests classes for different formats and special cases like date related tests. So should we maybe have them on just one test or 2-3 for different formats or so?

Wim Leers’s picture

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.
amateescu’s picture

@Wim Leers, ok, how about implementing @Berdir's suggestion and provide these methods only in 3 classes, for example EntityTestHalJsonAnonTest, EntityTestXmlAnonTest and EntityTestJsonAnonTest?

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?

Wim Leers’s picture

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.

Wim Leers’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 26: 2936704-26.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
33.22 KB
1.04 KB

#26 looks great to me. This should fix the fails.

Wim Leers’s picture

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

😱😅

STUPID STUPID STUPID Wim 😆

Thanks, Andrei!

alexpott’s picture

Adding credit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed d4e8416189 to 8.6.x and 0934fdc54a to 8.5.x. Thanks!

  • alexpott committed d4e8416 on 8.6.x
    Issue #2936704 by Wim Leers, amateescu, alexpott, Berdir: Remove REST...

  • alexpott committed 0934fdc on 8.5.x
    Issue #2936704 by Wim Leers, amateescu, alexpott, Berdir: Remove REST...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.