EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method

#2662284-31: Return complete entity after successful PATCH is another example.

#5: Sure, but that's of the least concern right now. Plus, there are no known bugs wrt REST + (Dynamic) Page Cache.

I've not started on it. And yes, one for every entity type, but not the two dozen or so "test" entity types. So, probably about a dozen unique tests. But look at e.g. \Drupal\node\Tests\NodeCacheTagsTest + \Drupal\comment\Tests\CommentCacheTagsTest + \Drupal\user\Tests\UserCacheTagsTest + … — there's almost zero code in there :)

This is blocked on #1841474: Convert REST tests to Guzzle (would also help with core moving from WTB to BTB). I'm going to be working on that issue, as well as this one in the coming days.

While #1841474: Convert REST tests to Guzzle (would also help with core moving from WTB to BTB) is waiting to be committed, I've already begun working on this one. This will not be finished before that one lands anyway.

I've got a start here now. This is testing the Node entity type against multiple formats (hal_json + json, xml is omitted for now — see next comment), and for now only with a single authentication mechanism (basic_auth). They follow a shared, standardized test scenario, that is specified in EntityResourceTestBase — which is the REST equivalent for the comprehensive cache tags test coverage at \Drupal\system\Tests\Entity\EntityCacheTagsTestBase.

For now, this only testing GET: it includes a superset of the test scenario that \Drupal\rest\Tests\ReadTest uses.

(The individual-commits patch is included to show how I got to this point. From now on, I'll omit that, every interdiff will correspond to a local commit.)

This is all that it takes to test an additional format. Pretty nice, is it not? :)

Except… this is something HEAD does not test. And so the very first thing I test that HEAD is not testing… is broken: the attached patch will fail. I opened an issue for it: #2800873: Add XML GET REST test coverage, work around XML encoder quirks.

Apparently I've had the base URL configured incorrectly in my unit tests for a very long time. Hence wrong code, hence #12 failed. This fixes it. Now you'll see the 2 fails in #13 more clearly.

I failed to include all my changes.

New URL-related fail. This time for the error message that we asse
rt. Apparently $entity->toUrl() generates a relative URL that is relative to the Drupal installation, and not relative to the domain. This should solve that.

(I lifted the solution from the CDN module for Drupal 8.)

I've been interpreting that error message incorrectly. Sorry for the noise. Fingers crossed.

There we are! Finally.

fail: [Other] Line 81 of core/modules/rest/src/Tests/NodeXmlBasicAuthTest.php:
Drupal\rest\Tests\NodeXmlBasicAuthTest::testGet
Symfony\Component\Serializer\Exception\UnexpectedValueException: A string must be provided as a bundle value.

This is what I alluded to in #13, where I wrote:

Except… this is something HEAD does not test. And so the very first thing I test that HEAD is not testing… is broken: the attached patch will fail. I opened an issue for it: #2800873: Add XML GET REST test coverage, work around XML encoder quirks.

So, for now, reverting the XML test coverage. We don't have that in HEAD either. And because we don't, we didn't know this didn't work. This patch/test coverage is already learning us what does and doesn't work, and it's only in the early stages!

No need to be sad, we are learning where our flaws are :)

Gah.

Moving all the things to the right places.

Now no longer testing only Node: added TermResourceTestBase, TermJsonBasicAuthTest and TermHalJsonBasicAuthTest.

Same mistake as in NodeResourceTestBase, which I fixed in #26. Same fix.

@dawehner: Thanks for the review! Will address in the next round.

While working on porting over user_role testing from HEAD's ReadTest, I made a mistake (I forgot to enable the hal module for a test that was testing hal_json, which caused its GET route to not exist). This caused me to run into another bug: the serializer.formats container parameter is used to create REST GET routes. So even if you only allow hal_json, it'll still create GET routes for json and xml (i.e. the default formats).

I predicted this bug almost 4 months ago at #2737751: Refactor REST routing to address its brittleness:

3. ResourceBase generates individual GET routes for every available serialization format, not just the ones that are allowed by the configuration, and then ResourceRoutes::alter() removes those that are not acceptable by the configuration.

Attached is a reroll that adds test coverage for user_role and block config entities.

So, the patch currently ports a superset of the test logic in ReadTest, for node, taxonomy_term, block and user_role. term wasn't tested before. That leaves entity_test, config_test and taxonomy_vocabulary to be ported to have all of the test coverage in ReadTest.

Now we're really getting up to speed :)

Updated the issue summary to list all the currently known/planned todos.

Since this is about providing test coverage to ensure that REST in Drupal 8.2 is as reliable as possible, this really should go in 8.2. This won't cause API changes. At most, it will cause bug fixes. Which is the whole point.

Marking the already completed items as completed in the IS.

I had it ready, but I failed to include a small change in #32 to correctly generate URLs for config entities. Here's the fix.

I have no idea what's going on in #37, because all those tests pass fine locally. It's the same assertion that (only) testbot keeps failing on in every scenario, but only when using basic_auth: instead of Drupal returning text/html; charset=UTF-8, it's returning text/plain; charset=UTF-8. No idea how that is possible.

For now, commenting that assertion, along with a @todo comment. Let's hope there's not another assertion further in the testGet() scenario that also fails…

So far, I've labeled all tests as "basic auth". But actually, in reality, they're testing the behavior in case of no authentication, i.e. the anonymous case. Given sufficient permissions to the anonymous user, this is also possible. Which makes sense, because some REST APIs are intended for public (authless) consumption.

After quite a struggle, I got that working!

In this reroll, every existing test is split up in a "basic auth" test and an "anon" test. Any contrib module that provides a new authentication mechanism can then provide similar, simple test classes with barely any code, and hence run the entire test suite!

The struggle is because of the weird interaction/design of the routing + authentication system, which causes things to fail miserably in an unfriendly way when the user forgets to specify ?_format=nonsense, or has a typo (for example ?_format=haljson on a URL that already has a HTML route.

It's very very very confusing how Symfony's routing system first grants access because a user is authenticated via basic auth, and so AccessAwareRouter says everything is okay, and then afterwards, during AuthenticationSubscriber::onKenrelRequestFilterProvider(), we choose to deny access after all because basic_auth isn't a globally allowed access provider, and the matched route (which matches the HTML, non-REST route due to the missing ?format=) is lacking an _auth route option. You can either get a 406 (which is what we want), or a 403 with HTML (in case of non-specified or invalid format) or a 403 in the expected format (in case of non-specified or invalid format plus Accept header, even though we don't actually support Accept headers… thanks to DefaultExceptionSubscriber calling Request::getAcceptableContentTypes()).
Worse, it's even possible to get the appropriate 406 response in case of an invalid format, but for that response to be sent with Content-Type: text/html! (In case of anonymous.)

More edge cases than I can count.

Seems like the authentication system and routing system are integrated in a brittle, confusing, backward manner. And the content negotiation system makes it that tiny bit extra unpredictable. This is no one's fault. This is complex software with (too) many layers, and without comprehensive integration tests, this sort of thing is to be expected.

So, the updated test coverage verifies this crazy behavior is at least consistent. And then when we fix it, in #2805279: Routing system + authentication system + format-specific routes (e.g. those in rest.module) = frustrating, unhelpful 403 responses instead of 406 responses, we can massively simplify this crazy test logic. It's better to embrace and accept the status quo, because that means we are better prepared to improve it :)

Finally, less important, but still a WTF: the error response body in case of missing Basic Authentication is different for the json and hal_json formats. In the former case, it's actually in JSON, but with a strange prefix, and in the latter case, it's in plain text… opened an issue for that too: #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json.

Finally^2 (I found this just before posting), I found another absolutely fascinating edge case. I'll let the comment in the test speak for itself:

    // All blocks can be viewed by the anonymous user by default. An interesting
    // side effect of this is that any anonymous user is also able to read the
    // corresponding block config entity via REST, even if an authentication
    // provider is configured for the block config entity REST resource! In
    // other words: Block entities do not distinguish between 'view' as in
    // "render on a page" and 'view' as in "read the configuration".
    // This prevents that.
    // @todo Investigate further.
    …

I just postponed #2135829: EntityResource: translations support on this — without this issue, that issue is very likely to introduce even more edge cases.

#39: Yes, the vast majority of phase 2 will definitely be handled by other issues. But as the issue summary says, this must provide at least parity with (i.e. a superset of) the existing tests. Which will then allow us to remove those tests.

Quoting myself in #40:

I have no idea what's going on in #37, because all those tests pass fine locally. It's the same assertion that (only) testbot keeps failing on in every scenario, but only when using basic_auth: instead of Drupal returning text/html; charset=UTF-8, it's returning text/plain; charset=UTF-8. No idea how that is possible.

For now, commenting that assertion, along with a @todo comment. Let's hope there's not another assertion further in the testGet() scenario that also fails…

Clearly, my fear came true. Sigh :(

What's interesting, is that 100% of the fails are for the hal_json tests. The tests for the json format are unaffected. Even more interesting: I observed in #41 that the 401 error response for the hal_json format marks itself as Content-Type: application/hal+json, yet the response body is plain text. Funny enough… on testbot it apparently does say Content-Type: text/plain, which is consistent with the response body!

I can't reproduce this when

1. using Drupal in the root of the domain (http://tres/) and
   1. running the test directly with PHPUnit
   2. running all --group hal tests with PHPUnit
   3. running the test directly with run-tests.sh
   4. running all --module hal tests with run-tests.sh
2. using Drupal in a subdirectory of a domain (http://localhost/tres/ and
   1. running the test directly with PHPUnit
   2. running all --group hal tests with PHPUnit
   3. running the test directly with run-tests.sh
   4. running all --module hal tests with run-tests.sh

So I'm completely out of ideas. I'm going to have to comment out all the Content-Type response header assertions. We'll need to sort this out before this eventually gets committed. I officially give up.

The final idea I had is that perhaps the response contains two Content-Type headers, and that somehow testbot parses the first one and my local installation parses the second one (or vice versa). But, no such luck:

HTTP/1.1 401 Unauthorized
Date: Mon, 26 Sep 2016 15:58:03 GMT
Server: Apache/2.2.29 (Unix) DAV/2 PHP/5.5.11 mod_ssl/2.2.29 OpenSSL/0.9.8zg
X-Content-Type-Options: nosniff
X-Powered-By: PHP/5.5.11
WWW-Authenticate: Basic realm="tres"
Cache-Control: must-revalidate, no-cache, private
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Content-Length: 39
Connection: close
Content-Type: application/hal+json

No authentication credentials provided.

So now that we have a decent starting point, moving the test coverage to the modules responsible for the biggest divergence, and hence the biggest reason for them to have explicit comprehensive test coverage: those providing additional formats.

In this reroll, I moved all hal_json tests (filenames matching *HalJson*Test.php) to the hal module. The https://www.drupal.org/project/jsonapi module will be able to basically copy/paste and rename these.

Added entity_test content entity test coverage. (Because ReadTest::testRead() is testing this, so we need it for test coverage parity.)

Whew! After a >24 hour detour on new bugs, here I am again with another update!

Added taxonomy_vocabulary config entity test coverage.

Discovered another problem: #2807263: Impossible to write unit tests involving Vocabulary, because TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE) are defined in taxonomy.module instead of VocabularyInterface. Which uncovered yet another problem, which also happens to improve anonymous REST response performance significantly: #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty.

So, the anonymous vocabulary tests have their GET test coverage disabled for now, until #2807263: Impossible to write unit tests involving Vocabulary, because TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE) are defined in taxonomy.module instead of VocabularyInterface lands.

Added config_test config entity test coverage.

This didn't uncover new bugs — hurray! But some special extra sauce was necessary to make config_test properly testable. See the tiny config_test_rest test-only module for why.

This marks the completion of porting 100% of the test coverage of ReadTest over to this new test coverage! It now not only does a superset of the test scenario, it now also tests all the entity types that ReadTest was testing.

Next: CreateTest.

The experience I gained with working on Vocabulary-related things for this issue, and the fact that I got the HTTP GET tests for it to pass in #49 allowed me to answer #2772413: REST GET fails on entity/taxonomy_vocabulary/{id} 403 Forbidden with error with confidence. The problem must've been somewhere else.

And sure enough, the problem turns out to have been that they did not grant the administer taxonomy permission. But of course, doing that would also be quite dangerous, if all you need is the ability to read vocabularies!

So, we need to solve that on two levels:

1. saner default access to Vocabulary entities: #2808217: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission (patch posted!)
2. better error responses, that tell you why you're getting a 403: #2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out

Those are the two newest child issues for this issue!

I've got the POST test coverage working (ported most of CreateTest). It took a lot of effort, and I uncovered lots of bugs. But, before getting to that point, and posting that interdiff+patch, I'm posting some refactoring that was necessary to get there. That'll make the POST test coverage easier to review.

1. Rename setUpAuthentication() to setUpAuthorization(): this was not handling authentication, but authorization: I named it poorly when I introduced it.
2. Rather than letting setUpAuthorization() create accounts, but only in case of non-anon tests, I moved that logic to ResourceTestBase. Less code in actual tests, so less that can go wrong. Now it only needs to grant permissions to either the anonymous or authenticated role.
3. Added ResourceTestBase::assertResourceResponse().
4. Renamed+moved EntityResourceTestBase::assertErrorResponse() to ResourceTestBase::assertResourceErrorResponse().
5. Added ResourceTestBase::grantPermissionsToTestedRole(), which calls grantPermissionsToAuthenticatedRole() when an authentication provider is being used, and calls grantPermissionsToAnonymousRole() otherwise. This allows us to remove the setUpAuthorization() method in every single concrete test class, and move it in the per-entity type base classes. So much simpler! This helps reduce patch size too :)

Discussed with dawehner in IRC.

1. This patch only needs very high-level review for now: test infrastructure, plus the scenarios (in EntityResourceTestBase).
2. I'm working towards that!

And here it is! The test of all the fundamental things in CreateTest (which tests POSTing of entities). This is not yet a superset of CreateTest.

Necessary to achieve superset:

1. the BC flag (also need to add this for GET test coverage)
2. field-level validation errors
3. X-CSRF-Token header testing (but this requires the cookie authentication provider, which I've yet to add test coverage for)
4. Comment entity type
5. User entity type

Already surpassing existing test coverage in that it tests Term and has far stricter assertions, hence stronger guarantees.

While working on that, I encountered no less than seven bugs/WTFs that resulted in new issues, and which slowed me down on the order of many days:

1. A confusing error message when forgetting to send the Content-Type request header: #2811133: Confusing error response set by ContentTypeHeaderMatcher when forgetting the Content-Type request header
2. #2817727: Add test coverage to prove controller is called *after* authentication validation
3. Inconsistently encoded JSON responses: #2813755: JSON responses encoded inconsistently: make them all RFC4627-compliant
4. Inconsistent error responses: #2813853: RequestHandler has its own error handling rather than leaving it to KernelEvents::EXCEPTION event subscribers
5. Manual route rebuilding required: #2815845: Importing (deploying) REST resource config entities should automatically do the necessary route rebuilding
6. Authentication WTF wrt anon: #2817737: A REST resource's "auth" configuration MUST be non-empty, which prevents configuring it for anon access only
7. Authentication WTF wrt cookies: #2817745: Add test coverage to prove that REST resource's "auth" configuration is also not allowing global authentication providers like "cookie" when not listed

I simply uploaded the wrong patch. They were listed just above each other, I ended up selecting the wrong one, because my computer is super laggy.

24 different failures this time. Failed because #2811133: Confusing error response set by ContentTypeHeaderMatcher when forgetting the Content-Type request header landed while I was working on this, which means one @todo in this patch can be fixed :)

Update testGet() to match the improvements in testPost().

Fixing mistake in IS: there's no test coverage yet for using the cookie authentication provider.

Test what happens before provisioning a REST resource and before granting the necessary authorizations. Both for testGet() and testPost().

Test BC permissions. Both for testGet() and testPost(). One less @todo!.

#65 contains a flaw that will cause failures for basic_auth tests (it will only work for anon===authless tests).

Apparently another fail sneaked in there. In all Block config entities. The root cause is that same problem that was discovered before (see #41):

    // All blocks can be viewed by the anonymous user by default. An interesting
    // side effect of this is that any anonymous user is also able to read the
    // corresponding block config entity via REST, even if an authentication
    // provider is configured for the block config entity REST resource! In
    // other words: Block entities do not distinguish between 'view' as in
    // "render on a page" and 'view' as in "read the configuration".

But #65 introduced additional assertions to ensure a 403 is returned before the necessary authorization is set up. The work-around I had in place didn't account for that. So, now I have changed the work-around to disallow access by any user until the necessary permission is granted. We still need to fix the root cause though.

Created an issue for that now: #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity".

Test coverage for the Comment entity, for parity with CreateTest.

#73:

1. Thanks, will do!
2. I'm just adopting the same approach/pattern as \Drupal\system\Tests\Entity\EntityCacheTagsTestBase::createEntity(). This is simply about creating an entity … and if it needs additional setup, then that too. I think "create entity" captures the spirit/intent best.
3. No, this is an assert(), not a test.
4. See 2.
5. You don't want to know how hard I tried not to use this ;) But because somebody decided it was a good idea to have two different ConfigTest entity types, with the exact same name, that is sadly impossible.
6. See 3.
7. No, that's not at all what the @todo is about. See the issue that it mentions: #2815845: Importing (deploying) REST resource config entities should automatically do the necessary route rebuilding. Once that issue is fixed, these drupal_flush_all_caches() calls can simply be removed. The only reason they're there is because after hours of debugging, that was the only solution that worked. They were introduced in #55.

I'm a bit concerned about test runtime. What is the runtime of all old REST tests vs. all the new Functional tests you introduced? Would be interesting to just get a feeling of how much we are adding here.

The old REST tests will be removed. Test run time is less important than developers (both core developers and front end developers actually using D8's REST API) wasting countless hours on bizarre errors. This test suite helps prevent that. And considering how many new issues I've already opened because of the test coverage written so far, it's definitely helping.

Can't say much about the overall approach with all the sub-classing,

I agree that the subclassing approach is clunky/verbose. But it's the only way that works: this is how we can test every entity type + format + authentication provider combination, and allow contrib modules that add more of either of those three to get the same solid test coverage. (It also means these tests can easily be run in parallel btw!)

the patch is a bit too large for my taste. I would argue to do this in smaller chunks, but whatever works for you.

+1000! I wish I could keep it smaller. But… this is a necessary evil when you're developing one generic test with scenarios that exercise all edge cases (EntityResourceTestBase's testGet() + testPost()). You need to have a sufficient variety of entity types to test against, because if you start with only a single one, then you'll need to keep modifying that base class in every issue where you add a new entity type, hence resulting in many conflicting patches, but also a less cohesive EntityResourceTestBase base class.

Note that I'm only including those entity types that are already covered in the existing REST test coverage. All others I'll defer to follow-ups!

I will address #73.1!

#72 added test coverage for Comment, but did not yet include test coverage for the insane edge cases/DX problems I encountered.

Opened an issue for that too: #2820364: Entity + Field + Property validation constraints are processed in the incorrect order.

Clean-up patch, that addresses #73.1 too. All of dawehner's feedback in #31 has already been addressed while posting all those intermediary steps.

Test coverage for the User entity, for parity with CreateTest.

#75 apparently had some issues. This should fix some of those fails. The ones that say Failed asserting that false matches expected true. are a bit problematic: I can't reproduce those locally. So I added additional debug output.

Okay… so this is yet another case of "DrupalCI has result X, locally it's result Y, and there's no way to debug DrupalCI". So I have to give up, and just make the assertions less strict.

CreateTest was asserting (for a few entity types) that a too long UUID resulted in a 404. This now does that for all entity types.

#85 introduced a failure because HAL's FieldNormalizer is very unforgiving when you use {field_name: 'abc'}: you must always specify {field_name: [{value: 'abc'}]}.

I opened #2820743: FieldNormalizers are very unforgiving, impossible to debug error response given for fixing that.

\Drupal\rest\Tests\CreateTest::testCreateEntityTest() was testing an access-protected configured field: a field that cannot be edited should result in a 403 and a response body with a meaningful message.

This brings that to EntityResourceTestBase and verifies this works for all entity types!

This adds test coverage for using the cookie authentication provider. It also adds the X-CSRF-Token header missing/invalid edge case DX testing to the POST test case scenario.

While working on this, I discovered a bug in the "login" RPC endpoint: #2820888: Cookie authentication: the user.login.http route never supports the 'hal_json' format or some other format, depending on module order.

This was the final thing to achieve superset of CreateTest, which means as of this patch, we have a superset of CreateTest's test coverage! Marking that one as done in the IS.

Next: UpdateTest + DeleteTest.

Previous patches introduced a bit of cruft, cleaning that up.

While working on testPatch() (for porting UpdateTest), I noticed that getNormalizedEntityToCreate() was a bit of an unfortunate name. This renames it to getNormalizedPostEntity(). Which then opens the door for getNormalizedPatchEntity().

Again while working on testPatch() (for porting UpdateTest), I noticed that verifyResponseWhenMissingAuthentication() was misnamed, it should have been named assertResponseWhenMissingAuthentication().

This ports the majority of UpdateTest. Some edge cases in UpdateTest still need to be brought over and generalized (particularly those for Comments, i.e. the coverage added in #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it). But in most respects, this is already testing a vast superset of what HEAD's test coverage does.

No new bugs found for once!

Bugs were found, I just forgot about that over the weekend. They are:

1. #2821013: EntityResource's field validation 403 message is inconsistent between POST and PATCH
2. #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors

This ports all of DeleteTest and in fact covers a superset already! :)

One more bug found, this time not in Drupal but in Symfony PHP! #2821711: Apache always sets Content-Type: text/html, even for DELETE requests

Thanks, will address these tomorrow! :)

#95:

1. You're right, I was using it inconsistently. You already commented on this in #31.5. I removed all the assertions in the getExpectedNormalizedEntity() implementations. I only kept two assertions: those in \Drupal\Tests\rest\Functional\ResourceTestBase::setUp(). I think it makes sense to use assert() there because it's asserting that the test environment itself is being set up correctly.
2. Done.
3. Done.
4. Oh, that'd be splendid! Except that it's probably a waste of your time to try to make a temporary work-around be less hacky — your time would be better spent fixing #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json, which would remove that trait altogether :)
5. It is an abstract method on ResourceTestBase :)
6. +1, done!
7. Indeed.
8. Of course, I'd love to have your review of this! This is indeed the absolutely essential part of this patch. It has a generic test scenario for GET, POST, PATCH and DELETE which is applied to every entity type. So that's how you need to look at this: as test scenarios that expose both a correct request and common mistakes. In case of a mistake, we verify that the response provides a helpful message.
9. I do not see how we can do that without making the scenarios impossible to follow. That's the whole point: each of those test methods is testing an entire scenario. It's already separated by HTTP method (GET/POST/PATCH/DELETE). Each of those scenarios follow the same pattern: start out without a provisioned REST resource, observe it fail, then provision, observe it fail due to lack of authorization, then grant authorization, then observe it fail due to incorrect request, etc. It always ends with a successful request.
   That being said, if you see a way to make this clearer without breaking up a scenario, then I'm all for it!
10. Yes, IMO we should deprecate \Drupal\rest\Tests\RESTTestBase.
11. :O I WISH I HAD KNOWN THIS! Thanks, improved that :)

Thanks again for your review! :) This patch is already better for it. Looking forward to your review of EntityResourceTestBase.

Per #94, DeleteTest was fully ported. Updating IS to reflect that.

Per #93, UpdateTest was mostly ported: all of the generic stuff was ported, but now there's still the specialized test coverage that needs to be ported (for example that added by #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it). I'm working on generalizing it.

Once that is done, I consider this issue done. I will file follow-ups to add test coverage for all other entity types. And then I'd like to add a test that ensures that every entity type has this test coverage at least for core's formats+auth providers, so that one can be certain that every entity type can in fact be interacted with via REST. (Look at \Drupal\Tests\help\Kernel\HelpEmptyPageTest::testEmptyHookHelp() for inspiration on how to do that.)

Of course, the cookie authentication provider's test coverage was also already completed, back in #89.

So, the 3 things that still need to be ported:

1. \Drupal\rest\Tests\UpdateTest::testPatchUpdate()'s Make sure that the field does not get deleted if it is not present in the PATCH request. portion exists in the new test coverage (the rest of it is already done, and more)
2. \Drupal\rest\Tests\UpdateTest::testUpdateUser()'s test coverage that asserts some fields can only be modified when given the current password.
3. \Drupal\rest\Tests\UpdateTest::testUpdateComment()'s test coverage for read-only fields (which BTW differs between HAL+JSON vs JSON…), which was introduced at #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it.

This reroll addresses the first one. In doing so, I stumbled upon two problems:

1. new: #2822611: Document why UserInterface + FileInterface + MenuLinkContentInterface + … extend \Drupal\Core\Entity\ContentEntityInterface — turns out this is by design…
2. pre-existing: #2808335-13: Changed time not updated when only non-translatable fields are changed on a translated entity, work-around included

DELETED (pasted my comment intended for another issue)

Small reroll for free beer: https://twitter.com/tstoeckler/status/791769722941480968.

This addresses #100.2:

2. \Drupal\rest\Tests\UpdateTest::testUpdateUser()'s test coverage that asserts some fields can only be modified when given the current password.

I ran into #2813755: JSON responses encoded inconsistently: make them all RFC4627-compliant once again while working on this one.

#104: :D

And this addresses #100.3:

3. \Drupal\rest\Tests\UpdateTest::testUpdateComment()'s test coverage for read-only fields (which BTW differs between HAL+JSON vs JSON…), which was introduced at #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it.

Doing so resurfaced the existing problems that were already documented in #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it. But thanks to the test comprehensive test coverage introduced here, it is much clearer/scarier now how the HAL normalization causes aberrant behavior. So, opened #2824271: HAL causes 'pid' and 'homepage' fields on Comment entity to _not_ be forbidden to send in PATCH requests to fix that.

A round of self-review.

1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
   @@ -0,0 +1,73 @@
   +trait HalEntityNormalizationTrait {
   

   Needs docs.

2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/HalEntityNormalizationTrait.php
   @@ -0,0 +1,73 @@
   +  protected function applyHalFieldNormalization(array $normalization) {
   

   Needs docs.

3. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Vocabulary/VocabularyHalJsonAnonTest.php
   @@ -0,0 +1,43 @@
   +   * Disable the GET test coverage due to bug in taxonomy module.
   +   * @todo Fix in https://www.drupal.org/node/2805281: remove this override.
   

   Merge these comments.

4. +++ b/core/modules/rest/tests/modules/rest_test/rest_test.module
   @@ -22,3 +22,24 @@ function rest_test_rest_relation_uri_alter(&$uri, $context = array()) {
   \ No newline at end of file
   

   Fix this.

5. +++ b/core/modules/rest/tests/src/Functional/AnonResourceTestTrait.php
   @@ -0,0 +1,22 @@
   +trait AnonResourceTestTrait {
   

   Add docs.

6. +++ b/core/modules/rest/tests/src/Functional/BasicAuthResourceTestTrait.php
   @@ -0,0 +1,36 @@
   +/**
   + * ResourceTestBase::getAuthenticationRequestOptions() for basic_auth.
   + */
   +trait BasicAuthResourceTestTrait {
   +
   

   Improve docs.

7. +++ b/core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php
   @@ -0,0 +1,115 @@
   +/**
   + * ResourceTestBase::getAuthenticationRequestOptions() for cookie.
   + */
   +trait CookieResourceTestTrait {
   

   Again.

8. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
   @@ -0,0 +1,295 @@
   +  public function testPostDxWithoutCriticalBaseFields() {
   

   Needs docs.

9. +++ b/core/modules/rest/tests/src/Functional/EntityResource/ConfigTest/ConfigTestResourceTestBase.php
   @@ -0,0 +1,72 @@
   +    $config_test = entity_create('config_test', [
   

   Document why this does not use ConfigTest::create().

10. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +abstract class EntityResourceTestBase extends ResourceTestBase {
    

    Add @see examples to these docs.

11. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +  protected static $entityType = NULL;
    

    s/$entityType/$entityTypeId/

12. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +  protected static $patchProtectedFields;
    

    s/Fields/FieldNames/

13. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +  protected static $labelField = NULL;
    

    s/Field/FieldName/

14. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +  abstract protected function createEntity();
    ...
    +  abstract protected function getExpectedNormalizedEntity();
    ...
    +  abstract protected function getNormalizedPostEntity();
    

    Mention these in the class docblock.

15. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -0,0 +1,922 @@
    +  public function testGet() {
    ...
    +  public function testPost() {
    ...
    +  public function testPatch() {
    ...
    +  public function testDelete() {
    

    Add docblocks to each of these, and ensure there's no other methods listed in between these.

16. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestJsonBasicAuthTest.php
    @@ -0,0 +1,46 @@
    +  }
    +
    +
    +}
    

    Two newlines, should be one.

17. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestResourceTestBase.php
    @@ -0,0 +1,127 @@
    +    $entity_test = EntityTest::create(array(
    

    s/array()/[]/

18. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    @@ -0,0 +1,140 @@
    +        // @todo Create issue similar to https://www.drupal.org/node/2808217.
    +        $this->grantPermissionsToTestedRole(['administer taxonomy']);
    

    Actually create this issue.

19. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
    @@ -0,0 +1,336 @@
    +/**
    + * Subclass this for every REST resource, every format and every auth mechanism.
    + */
    +abstract class ResourceTestBase extends BrowserTestBase {
    +
    

    The docblock is both wrong and incomplete. Rewrite.

Addressed that. Opened #2824408: To be able to create/update/delete Term entities via REST, one should not have to grant the 'administer taxonomy' permission for point 18.

First round of issue summary update: to reflect the current status and scope. Scope-related follow-up created: #2824572: Write EntityResourceTestBase subclasses for every other entity type..

Second round of issue summary update: to reflect what to do with the old tests. Related follow-up created: #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase.

Now all necessary follow-ups have been created.

Third and final round of issue summary update. Added more context for reviewers & committers.

Fix typo.

While working on #2824576, I found one more thing that makes sense to assert: more cacheability aspects. The cache tags + contexts for the GET/HEAD response. And the absence of the X-Drupal-Cache header for unsafe methods (POST/PATCH/DELETE).

See #2824576-2: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase, point 4.

This adds those.

And also while working on #2824576, there is one last thing I found that makes sense to add to the test coverage here. I missed it because we have NodeTest which duplicates big portions of (Read|Create|Update|Delete)Test. But the portions it doesn't duplicate, is for asserting the error response when the developer forgets to specify the bundle for the entity (or specifies an incorrect one). This is a common, easy mistake, and it's very much worth testing that this works correctly for all entity types, to ensure a good DX/RX.

See #2824576-2: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase, point 3.

This adds that test coverage. In doing so, I generalized it, so that it doesn't run just for Node entities using the json format, but for all entity types (that have bundles) and for all formats. This led me to discover that the hal normalization actually breaks ungracefully when you specify an invalid bundle (it fails gracefully when omitting a bundle completely): with a PHP error and without useful DX feedback. Opened #2824827: \Drupal\hal\Normalizer\ContentEntityNormalizer::denormalize() fails with fatal PHP error when bundles are missing from link relation types for that.

And with that, this is 100% ready for final review.

Tagging with RX & DX, since this test coverage will help prevent regressions in RX & DX.

1. There's lots of these already, see \Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator::getDefinition(), \Drupal\Core\Access\AccessResult::isAllowed, \Drupal\Core\Datetime\Element\Datelist::valueCallback(), and so on. 290 occurrences of the regex \* \{\@inheritdoc\}\n \*\n \*
2. I don't think I can add a meaningful comment to this. Just like there are no comment docblocks on NodeResourceTestBase etc, because it's simply overkill/unnecessary.
3. See 1.
4. Fixed!
5. Well… that's just an entity normalization. Normalization arrays always have meaningful keys.
6. Heh :)
7. Fixed!
8. Oh, great catch, thanks, fixed!
9. Fixed!
10. Fixed!
11. Fixed!
12. Fixed!
13. Fixed!
14. See 1.
15. Fixed!
16. Another great catch — fixed!
17. Fixed!
18. Fixed!
19. Fixed!
20. This is intentional, for legibility. You'll see the same style in EntityResourceTestBase::test(Get|Post|Patch|Delete)(). And you'll see that it's very necessary there.
21. Ah, yes, of course! I missed this one. Fun fact: I already created this issue two weeks ago, just forgot to update this comment :) #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity"
22. Fixed!
23. Fixed!
24. Fixed!
25. Fixed!
26. Fixed!
27. Done, and also updated test(Post|Patch|Delete)()
28. Fixed!

Filed issues for the remaining things I encountered (for which I had commits in my local branch, to address later):

1. #2825347: 'Accept' header still is used for determining response format of error responses, ?_format= has no effect AND serialization module's exception subscriber does not support status code 500 error responses
2. #2826391: CsrfAccessCheck should have proper error feedback for invalid/missing CSRF token query argument just like CsrfRequestHeaderAccessCheck
3. #2826407: PATCH + POST allowed format validation happens in RequestHandler::handle(), rather than in the routing system

Final round of self-review & clean-up:

1. went over every single remaining @todo to ensure they're correct. Two didn't have issues. One of those two I was able to remove+address. And the other I was able to simplify and link to an issue I had created prior (#2813755: JSON responses encoded inconsistently: make them all RFC4627-compliant).
2. I added a @todo or #118.1, in the place that made me discover it.
3. I added test coverage for #118.3.
4. Fixed one inconsistency.

1. You're right, that comment is weird. Fixed.
2. Yes! This was very tricky to figure out, but using getOwner() meant we were calling \Drupal\user\Entity\User::getAnonymousUser(), which generates an in-memory User entity. Which has no UUID set, which meant that the code further down that sets the expected _links normalization would fail, because it expects ['uuid' => ['value' => NULL]], which is of course wrong. See \Drupal\comment\Entity\Comment::getOwner(). Documented this now.
3. Nice catch, done.
4. I considered this, but I like this approach, because it makes the requirements for an authentication provider very explicit. You can currently easily compare \Drupal\Tests\rest\Functional\CookieResourceTestTrait::getAuthenticationRequestOptions() with \Drupal\Tests\rest\Functional\BasicAuthResourceTestTrait, and see what exactly is expected of each request. Contrib modules like simple_oauth will add their own, and it'll again be clear to compare. If we start using Guzzle's built-in "cookie tracking", then we lose that explicitness, that clarity.
5. Good point. We should also allow TRACE then. See https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html. Note this is exactly what Django does too: https://docs.djangoproject.com/en/dev/ref/csrf/#how-it-works.
6. Feeling free to ignore; I've done it this way for consistency with EntityResourceTestBase::test(Get|Post|Patch|Delete)()
7. :) Glad you like it!
8. Great catch, improved.
9. Makes sense to me. Done.
10. Yes, it's needed. explode(' ', '') === [''], sadly.
11. Agreed, good find. Fixed.
12. Again feeling free to ignore, same reason as point 6.
13. Oh, yes, lovely!
14. Yep, also fixed.
15. :) I think it's a requirement for something that's the actual interface, the actual API. How else would we know that we break BC?
16. If that's the case, then \Drupal\serialization\Normalizer\EntityNormalizer::denormalize() is also wrong, and we'd need to fix that. (In a separate issue of course, since this one is test-only.) Note how it does:
    

        $entity_type_definition = $this->entityManager->getDefinition($entity_type_id, FALSE);
        …
        if ($entity_type_definition->hasKey('bundle')) {
          …
          $bundle_type_id = $entity_type_definition->getBundleEntityType();
          $bundle_types = $bundle_type_id ? $this->entityManager->getStorage($bundle_type_id)->getQuery()->execute() : [];
          …
          // Make sure a bundle has been provided.
          if (!is_string($data[$bundle_key])) {
            throw new UnexpectedValueException('A string must be provided as a bundle value.');
          }
    
          // Make sure the submitted bundle is a valid bundle for the entity type.
          if ($bundle_types && !in_array($data[$bundle_key], $bundle_types)) {
            throw new UnexpectedValueException(sprintf('"%s" is not a valid bundle type for denormalization.', $data[$bundle_key]));
          }
    

    (i.e. any entity type definition that has a bundle key can have that bundle type unexpected error response occur, as long as there is a bundle entity type)
    … oh but wait, it's still possible to get the A string must be provided as a bundle value error response even when there is no bundle entity type! So you're partially right :) Or maybe even entirely right — it's not clear what everything was that you were pointing at.

17. That's a very good point! I should've made the same remark :) Created an issue: #2827084: EntityNormalizer::denormalize should not throw UnexpectedValueException, but \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException, so we get a 422 response instead of 400. And added it as a @todo.
18. Heh…
19. :) Clear, isn't it? :)
20. :D Fixed, here and everywhere else.