Simplify REST configuration

+1 for principle. But we need to better understand whether we would do this before #2308745: Remove rest.settings.yml, use rest_resource config entities, or after, or only do this.

So, you're proposing this order:

1. #2721595: Simplify REST configuration (this issue)
2. #2308745: Remove rest.settings.yml, use rest_resource config entities

Right?

Ok.

This patch fully implements this simplified REST configuration as discussed & agreed by @klausi, @dawehner and I at #2308745: Remove rest.settings.yml, use rest_resource config entities, see comments #170–#202.

This patch is built on top of #2308745-208: Remove rest.settings.yml, use rest_resource config entities. It already includes upgrade path test coverage (with an upgrade path relative to that of #2308745).

git is stupid, it really really wants to consider that PHP file as binary, even though .gitattributes already tells/forces it otherwise…

So, rolling the patch with --binary, because I can't figure out how to force git to be sane.

First, rebased on top of #2308745-213: Remove rest.settings.yml, use rest_resource config entities. Should have the same fails.

#11: fixed. That will hopefully fix the first fail. I couldn't reproduce that fail locally, so I can't be certain.

The second fail — which is reproducible — is now also fixed. But I fixed it in a perhaps unexpected way. The test itself is bizarre: it installs the hal and rest module, which means we don't even need to enable json. We just need to enable cookie authentication on the existing resource. And we just need to request it in HAL+JSON, rather than JSON. That seems closer to the original intent.

Self-review:

1. +++ b/core/modules/rest/tests/fixtures/update/rest.resource.entity__node_2721595.yml
   @@ -0,0 +1,29 @@
   +    - node
   

   Should be user.

2. +++ b/core/modules/rest/tests/fixtures/update/rest.resource.entity__user_2721595.yml
   @@ -0,0 +1,30 @@
   +      - oauth
   

   Specifying cookie would be saner, since that actually exists.

3. +++ b/core/modules/rest/tests/fixtures/update/rest.resource.entity__user_2721595.yml
   @@ -0,0 +1,30 @@
   +    - node
   

   Should be user.

#2308745-213: Remove rest.settings.yml, use rest_resource config entities introduced 5 new failures. #2308745-216: Remove rest.settings.yml, use rest_resource config entities fixed them. No interdiff because this is just a rebase on top of 216.

#2308745: Remove rest.settings.yml, use rest_resource config entities landed, now rerolling this.

Rebased.

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

SIGH. git diff --binary it is.

One small change in ResourceGranularityRestResourcesConfigEntitiesUpdateTest wasn't committed. Can't provide an interdiff because git insists on interpreting this file as binary — see #21.

And this fixes the fails in ConfigDependenciesTest. This also made me realize we need expanded test coverage for that. But first, let's see whether this is green already.

YAY! Green! Now working on tests and clean-up.

Yep, that's what I'm working on :)

I don't mind rerolling, don't worry :)

Nicely triggered :D

Here's the missing test coverage. It only made sense to update 2 test existing methods to accept providers, the rest requires separate test methods for "method" vs "resource" to be able to sanely test things. In doing so, updated the onDependencyRemovalForResourceGranularity() code to be in sync with changes made to the parent issue after it was split off.

Now hunting for inconsistencies.

GAAAHHHHHH git diff --binary!!!!

1. +++ b/core/modules/rest/rest.install
   @@ -40,5 +42,39 @@ function rest_update_8201() {
   +function rest_update_8202() {
   

   This actually belongs in post-update.

   I started working on that, but… quickly ran into problems:

   18:35:09 <WimLeers> dawehner: alexpott_ I think I've found a significant problem with hook_update_post_NAME(), i.e. https://www.drupal.org/node/2563673
   18:35:10 <Druplicon> https://www.drupal.org/node/2563673 => hook_post_update_NAME() introduced to change content after hook_update_N() runs => 0 comments, 4 IRC mentions
   18:35:23 <WimLeers> dawehner: alexpott_ It's not possible to test hook_post_update_NAME() in isolation
   18:35:28 <WimLeers> dawehner: alexpott_ they always run
   18:35:45 <WimLeers> dawehner: alexpott_ So in the "simplification" follow-up, I need to update the _existing_ tests!
   18:36:01 <WimLeers> dawehner: alexpott_ And every single future hook_post_update_NAME() will have to do the same
   

   So, postponing doing this until I get feedback about that.

2. +++ b/core/modules/rest/rest.install
   @@ -40,5 +42,39 @@ function rest_update_8201() {
   +    if ($resource_config_entity->get('granularity') === 'method') {
   ...
   +        $resource_config_entity->set('granularity', 'resource');
   

   Should use the constant.

3. +++ b/core/modules/rest/rest.install
   --- a/core/modules/rest/src/Entity/ConfigDependencies.php
   +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
   

   This looks bizarre when searching for classes. I wonder if renaming from ConfigDependencies to EntityResourceConfigDependencies would make more sense?

4. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
   @@ -16,6 +16,11 @@
   +   * Granularity value for per-resourceconfiguration.
   

   Typo.

5. +++ b/core/modules/rest/src/Tests/Update/ResourceGranularityRestResourcesConfigEntitiesUpdateTest.php
   @@ -0,0 +1,70 @@
   +class ResourceGranularityRestResourcesConfigEntitiesUpdateTest extends UpdatePathTestBase {
   ...
   +  public function testResourcesConvertedToConfigEntities() {
   

   Copy/paste remnant.

6. There's a @todo referencing this issue in rest.post-update.php. That should be removed.

For resource granularity REST resources, we could actually simplify the schema further. Looking into that now.

For resource granularity REST resources, we could actually simplify the schema further. Looking into that now.

I thought we could get rid of the configuration key for the resource granularity. We can't. We need the schema type (called granularity here) to affect a predictable key (called configuration here).

For #35.1, here's what I started to do: interdiff-post_update.txt.

The only remaining thing is #35.3. But that can happen in a follow-up for sure.

So, AFAICT the main thing to review here is whether to use hook_post_update_NAME() or hook_update_N().

#35 is green. This is now blocked on review. Unassigning.

Discussed with dawehner. Moving the upgrade path to post-update.

#40:

1. If we move to post-update, then this becomes moot.
2. Agreed!
3. Oh wait, what? I thought this was just a naming convention to create fixtures for a particular hook_update_N(), where the fixture must include the "N" in the filename?

Got it, thanks! :)

So this reroll:

1. moves the upgrade path from hook_update_N() to hook_post_update_NAME()
2. which means that the upgrade path test at RestConfigurationEntitiesUpdateTest had to be updated, because that will now also run this new post-update function
3. but this still adds a separate test, so we can test all edge cases of the upgrade path

To be able to test this post-update hook in isolation, which is necessary to be able to test all nuances of this post-update hook, turns out you have to set the existing_updates key-value pair in the post_update collection in the key-value store. This is the very first test doing that. That required a lot of digging.

However, to make it much worse, the move from hook_update_N() to hook_post_update_NAME() introduced at least two bizarre problems, one of which is minor, the other one of which is critical:

1. the minor one: loadMultiple() now appears to be insisting on returning things in a different order, even when calling resetCache() after running updates:
   

   $this->assertIdentical(['entity.comment', 'entity.node', 'entity.user'], array_keys($resource_config_entities));
   

   had to be changed to:

   $this->assertIdentical(['entity.node', 'entity.comment', 'entity.user'], array_keys($resource_config_entities));
   

2. the critical one: the test coverage now fails … because loadMultiple() seems to be forever returning the old values (even after calling resetCache()), and load() always returns the correct, updated value. This is a HUGE WTF. I'm totally lost. I included the following debug code:
   

       // WTF: loadMultiple() vs load() are returning different results!
   
       // prints 'method'
       debug($resource_config_storage->loadMultiple()['entity.node']->get('granularity'));
       // prints 'resource'
       debug($resource_config_storage->load('entity.node')->get('granularity'));
   
   
   
       // WTF++: even after resetting caches
       $resource_config_storage->resetCache();
   
       // prints 'method'
       debug($resource_config_storage->loadMultiple()['entity.node']->get('granularity'));
       // prints 'resource'
       debug($resource_config_storage->load('entity.node')->get('granularity'));
   

Help very much appreciated with solving that second WTF.

Because that of course means the tests aren't passing… the very same tests that were passing just fine before! That's the even more scary/mysterious part: that this bug apparently only happens when running/testing post-update functions.

Still to do:

1. sort out this huge WTF described above.
2. rename drupal-8.rest-rest_update_8202.php, since it's no longer for a hook_update_N() function

Rarely have I been this glad to see a fail from testbot. It confirms what I see locally:

debug: [Debug] Line 74 of core/modules/rest/src/Tests/Update/ResourceGranularityRestResourcesConfigEntitiesUpdateTest.php:
method

debug: [Debug] Line 76 of core/modules/rest/src/Tests/Update/ResourceGranularityRestResourcesConfigEntitiesUpdateTest.php:
resource

debug: [Debug] Line 84 of core/modules/rest/src/Tests/Update/ResourceGranularityRestResourcesConfigEntitiesUpdateTest.php:
method

debug: [Debug] Line 86 of core/modules/rest/src/Tests/Update/ResourceGranularityRestResourcesConfigEntitiesUpdateTest.php:
resource

Those should all be resource, but clearly they're not.

Hopefully a (config) entity system or update system expert can figure out the root cause for this.

We use simply a bad setup.

GAHHHHHHHHHHH !!!!!!!!!!!!!!

-     'name' => 'rest.resource.entity__node',
+     'name' => 'rest.resource.entity.node',

Glad you found it, a fresh pair of eyes clearly saved the day there :D

Here is a patch which adds a reset, but I'm not sure its actually a good idea to call out to any kind of api before running the test.

There's no reset in the patch.

Here is a patch which adds a reset, but I'm not sure its actually a good idea to call out to any kind of api before running the test.

IMO it is fine. The whole point of post-update is that it behaves just like regular code. Which means you get tag-based invalidation, and static cache invalidation.

This patch is now ready as far as I'm concerned. The only thing that remains to be done, is renaming core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_update_8202.php to something that doesn't include 8202 in its name. The post-update function has a quite long name (rest_post_update_simplify_method_granularity_to_resource_granularity_where_possible()), do we want the same for the fixture? Do we even want that very long name for the post-update function?

Cool, will do.

Done.

Random fail in Drupal\field\Tests\EntityReference\EntityReferenceAdminTest. Re-testing, back to RTBC.

Trivial conflict in core/modules/rest/config/optional/rest.resource.entity.node.yml after #2755843: The order in which config is saved affects dependency calculations landed.

Straight reroll, so no interdiff.

I'm getting an error trying to apply this update.

STR:
* Clean Drupal 8.1.6 install with Rest UI contrib
* Enable the user rest resource, with GET and basic auth
* Update core code to 8.2.x-dev
* Attempt to run update.php
* Get this:

rest module

Update #8201

Failed: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "rest_resource_config" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 125 of /Users/george.cassie/Sites/devdesktop/drupal-8.1.6/core/lib/Drupal/Core/Entity/EntityTypeManager.php).

#61: the same problem was reported at #2308745-253: Remove rest.settings.yml, use rest_resource config entities, it's not something this patch introduces.

Grrr. d.o. ate my dreditor review of this patch. I need to rewrite that review, but don't have time this second. Will do so within 24 hours.

1. +++ b/core/modules/rest/config/optional/rest.resource.entity.node.yml
   @@ -7,25 +7,14 @@ dependencies:
   -  GET:
   -    supported_formats:
   -      - hal_json
   -    supported_auth:
   -      - basic_auth
   -  POST:
   -    supported_formats:
   -      - hal_json
   -    supported_auth:
   -      - basic_auth
   -  PATCH:
   -    supported_formats:
   -      - hal_json
   -    supported_auth:
   -      - basic_auth
   -  DELETE:
   -    supported_formats:
   -      - hal_json
   -    supported_auth:
   -      - basic_auth
   +  methods:
   +    - GET
   +    - POST
   +    - PATCH
   +    - DELETE
   +  formats:
   +    - hal_json
   +  authentication:
   +    - basic_auth
   

   Yay!

2. +++ b/core/modules/rest/rest.post_update.php
   @@ -34,6 +31,42 @@ function rest_post_update_create_rest_resource_config_entities() {
   +          'formats' => $configuration['GET']['supported_formats'],
   +          'authentication' => $configuration['GET']['supported_auth']
   

   Is it not possible for existing config to support some methods without supporting GET?

3. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
   @@ -61,31 +61,21 @@ public static function create(ContainerInterface $container) {
   +      $methods = ['GET'];
   

   Same question here, but for new config using resource granularity.

4. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
   @@ -45,7 +45,7 @@ class RestResourceConfig extends ConfigEntityBase implements RestResourceConfigI
   +   * Currently either 'method' or 'resource'.
   

   Should we reference the constants here instead of the literals?

5. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
   @@ -116,8 +116,7 @@ public function getMethods() {
        else {
   -      throw new \InvalidArgumentException("A different granularity then 'method' is not supported yet.");
   -      // @todo Add resource-level granularity support in https://www.drupal.org/node/2721595.
   +      return $this->configuration['methods'];
        }
   

   Can we changes this and similar code blocks to use switch/case, and still throw an exception when not one of the allowed constants? Both for clarity, and since nothing in the config schema enforces the value to one of these 2.

6. +++ b/core/modules/rest/src/Tests/Update/RestConfigurationEntitiesUpdateTest.php
   @@ -55,8 +53,12 @@ public function testResourcesConvertedToConfigEntities() {
   diff --git a/core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php b/core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php
   

   This shows up in the patch as binary rather than text, so no additional context via dreditor. But, within this file, there's db ->merge statements, but we know the starting database, so shouldn't these all be either ->insert or ->update? Other similar fixtures in HEAD use explicit insert/update, not merge.

7. +++ b/core/modules/rest/tests/fixtures/update/rest.resource.entity.comment_2721595.yml
   @@ -0,0 +1,30 @@
   +      - xml
   

   Let's add a comment here explaining why this is done (e.g., so that there's a test of updating configuration with method-specific formats).

8. +++ b/core/modules/rest/tests/fixtures/update/rest.resource.entity.user_2721595.yml
   @@ -0,0 +1,30 @@
   +      - oauth
   

   Let's add a comment here explaining why this is done (e.g., so that there's a test of updating configuration with method-specific authentication).

Will reroll.

2. Interesting point! Technically that totally is possible. Fixed.
3. Also fixed.
4. Changed to reference the constants.
5. Done. (Note that config schemas cannot enforce the value to one of these 2: config schemas have no "enum" concept. Maybe that's something worth adding in the future…)
6. Other fixtures also use ->merge(), that's where I even got the idea from. Examples: core/modules/rest/tests/fixtures/update/rest-export-with-authentication.php, core/modules/system/tests/fixtures/update/drupal-8.language-enabled.php.
7. Done.
8. Done.

Re-RTBC'd since this is all trivial refactoring.

Random fail: UpdatePathTestBaseFilledTest .

Sorry. A few more nits.

1. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
   @@ -102,6 +96,10 @@ protected function calculateDependenciesForMethodGranularity(RestResourceConfigI
   +    if (isset($dependencies['module'])) {
   +      sort($dependencies['module']);
   +    }
   

   Either we assume that the result of this function always passes through DependencyTrait::addDependency() (which it does within RestResourceConfig::calculateDependencies()) prior to being saved to storage or otherwise needing to be sorted, or we do not assume this. If we assume it, then we shouldn't need this hunk, but we should add docs to the function to clarify this assumption. If it's only to make a unit test pass, then perhaps the unit test should be the one to sort. If we do not assume this, and therefore, require this function to sort, then it should mimic DependencyTrait::addDependency() and also ensure uniqueness.

2. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
   @@ -183,7 +182,71 @@ protected function onDependencyRemovalForMethodGranularity(RestResourceConfigInt
   +          if (in_array($format, $rest_config->getFormats('GET'))) {
   +            $configuration['formats'] = array_diff($configuration['formats'], $removed_formats);
   +          }
   

   Per #64.2, we shouldn't assume GET is defined. Technically, this code doesn't, as the implementation of getFormats() ignores the passed-in method, but that's not obvious from reading this code in isolation. Any reason not to simply use $configuration['formats'] instead of $rest_config->getFormats('GET')?

3. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
   @@ -183,7 +182,71 @@ protected function onDependencyRemovalForMethodGranularity(RestResourceConfigInt
   +          if (in_array($auth, $rest_config->getAuthenticationProviders('GET'))) {
   +            $configuration['authentication'] = array_diff($configuration['authentication'], $removed_auth);
   +          }
   

   Same as above, but for getAuthenticationProviders().

As I looked more at #69, I couldn't find a reason for those points to block commit, so pushed to 8.2.x. A follow-up for #69 would be appreciated though.

Awesome, thank you! I will file a follow-up first thing tomorrow!

Follow-up created: #2773185: Fix nits in \Drupal\rest\Entity\ConfigDependencies.

Also updated https://www.drupal.org/developing/api/8/rest: https://www.drupal.org/node/2667736/revisions/view/9794157/9913841.