Problem/Motivation

rest module's default settings reference the entity:node entity type, but rest.info.yml doesn't require node module.

Snippet from the default coGonfig:

resources:
  entity:node:
    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

Proposed resolution

Utilize a configuration entity type for configuring REST endpoints and provide an optional configuration entity that configures a REST endpoint for nodes. Tis makes it possible

Remaining tasks

  1. Switch to config entities for REST endpoint configuration
  2. Fix HEAD tests
  3. Incorporate test changes from earlier patches
  4. Use PluginCollections for easy dependency generation and plugin instantiation
  5. Try to move some methods from the Rescource Plugin onto the Configuration entity
  6. Decide whether to use the elaborate dependency generation code from #2308745-103: Remove rest.settings.yml, use rest_resource config entities or to rely only on resource plugin dependencies or maybe find another solution
  7. Decide what endpoints should be shipped with the REST module as optional configuration or maybe even moved to their respective modules

Tasks after the comment:

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because with the current design in HEAD configuration installation for rest.settings fails if the node module ins not enabled. It also fails if an install profile provides a custom rest.settings.yml which contains configuration for entity endpoints that are provided by contrib modules (see #2397271: REST configuration fails if it contains a plugin reference that does not exist).
Issue priority Major because it does not happen in general but only if the rest module is used and a custom rest.settings.yml is provided.
Disruption Disruptive for contributed and custom modules/install-profiles ond for other REST focused core issues because the API for managing REST endpoits changes.

User interface changes

None

API changes

Removed the simple configuration rest.settings.yml.
REST endpoints are now managed via a config entity.

E.g. creating an endpoint:

// if plugin id is for a derivative plugin replace with : because those are not valid in config entity id's
endpoint_id = str_replace(':', '__', $resource_plugin_id);
// get entity by id
/** @var \Drupal\rest\RestEndpointInterface $endpoint */
$endpoint = $this->endpoint_storage->create(['id' => $endpoint_id]);

$endpoint->addSupportedFormat('GET',  'hal_json');
$endpoint->addSupportedAuthenticationProvider('GET', 'basic_auth');

$endpoint->addSupportedFormat('POST',  'hal_json');
$endpoint->addSupportedAuthenticationProvider('POST', 'basic_auth');

$endpoint->addSupportedFormat('PATCH',  'hal_json');
$endpoint->addSupportedAuthenticationProvider('PATCH', 'basic_auth');

$endpoint->addSupportedFormat('DELETE',  'hal_json');
$endpoint->addSupportedAuthenticationProvider('DELETE', 'basic_auth');

$endpoint->save();

Also resource plugins can implement DependentPluginInterface, which make those dependencies appear on the config entity.

CommentFileSizeAuthor
#248 2308745-248.patch85.78 KBalexpott
#248 243-248-interdiff.txt4.74 KBalexpott
#243 2308745-243.patch84.94 KBalexpott
#243 241-243-interdiff.txt3.78 KBalexpott
#241 238-241-interdiff.txt12.35 KBalexpott
#241 2308745-241.patch83.21 KBalexpott
#238 interdiff.txt19.96 KBdawehner
#238 2308745-238.patch82.1 KBdawehner
#233 interdiff.txt691 bytesdawehner
#233 2308745-233.patch81.92 KBdawehner
#231 interdiff.txt4.68 KBdawehner
#231 2308745-231.patch81.91 KBdawehner
#228 interdiff.txt623 bytesdawehner
#228 2308745-228.patch81.92 KBdawehner
#223 interdiff.txt31.38 KBdawehner
#223 2308745-222.patch81.96 KBdawehner
#217 interdiff.txt1.05 KBdawehner
#217 interdiff.txt0 bytesdawehner
#217 2308745-217.patch75.57 KBdawehner
#216 interdiff.txt679 bytesWim Leers
#216 rest_settings_config_entity-2308745-216.patch75.49 KBWim Leers
#213 interdiff.txt1.1 KBWim Leers
#213 rest_settings_config_entity-2308745-213.patch74.94 KBWim Leers
#212 interdiff.txt14.7 KBWim Leers
#212 rest_settings_config_entity-2308745-212.patch73.92 KBWim Leers
#208 interdiff.txt3.41 KBWim Leers
#208 rest_settings_config_entity-2308745-208.patch68.94 KBWim Leers
#207 interdiff.txt5.75 KBWim Leers
#207 rest_settings_config_entity-2308745-207.patch67.36 KBWim Leers
#206 interdiff-206.4.txt11.51 KBWim Leers
#206 interdiff-206.3.txt9.7 KBWim Leers
#206 interdiff-206.2.txt7.19 KBWim Leers
#206 interdiff-206.1.txt4.03 KBWim Leers
#206 interdiff.txt22.23 KBWim Leers
#206 rest_settings_config_entity-2308745-206.patch62.8 KBWim Leers
#203 interdiff.txt2.25 KBWim Leers
#203 rest_settings_config_entity-2308745-203.patch65.44 KBWim Leers
#200 interdiff.txt6.87 KBWim Leers
#200 rest_settings_config_entity-2308745-200.patch65.73 KBWim Leers
#197 interdiff.txt1010 bytesWim Leers
#197 rest_settings_config_entity-2308745-197.patch63.99 KBWim Leers
#196 interdiff.txt1.63 KBWim Leers
#196 rest_settings_config_entity-2308745-196.patch64.12 KBWim Leers
#195 interdiff.txt595 bytesWim Leers
#195 rest_settings_config_entity-2308745-195.patch64.44 KBWim Leers
#191 2308745-191-example-do-not-test.patch917 bytesWim Leers
#191 2308745-191-simple_or_granular-do-not-test.patch1.42 KBWim Leers
#185 2308745-185.patch63.68 KBdawehner
#182 interdiff.txt8.49 KBdawehner
#182 2308745-182.patch63.39 KBdawehner
#179 interdiff.txt2.67 KBdawehner
#179 2308745-179.patch65.69 KBdawehner
#174 interdiff.txt5.23 KBdawehner
#174 2308745-174.patch65.64 KBdawehner
#169 interdiff.txt2.51 KBdawehner
#169 2308745-169.patch65.22 KBdawehner
#167 interdiff.txt37.18 KBdawehner
#167 2308745-166.patch64.8 KBdawehner
#163 interdiff.txt609 bytesdawehner
#163 2308745-163.patch67.88 KBdawehner
#161 interdiff.txt6.29 KBdawehner
#161 2308745-161.patch67.87 KBdawehner
#159 2308745-159.patch61.51 KBdawehner
#149 rest-2308745-149.interdiff.txt1.24 KBArla
#149 rest-2308745-149.patch61.55 KBArla
#147 rest-2308745-147.interdiff.txt22.8 KBArla
#147 rest-2308745-147.patch60.55 KBArla
#134 interdiff.txt29.96 KBdawehner
#134 2308745-134.patch59.13 KBdawehner
#134 interdiff.txt23.44 KBdawehner
#134 2308745-133.patch59.1 KBdawehner
#129 rest-node-dep-2308745-129.patch59.46 KBg.oechsler
#129 interdiff.txt8.71 KBg.oechsler
#127 rest-node-dep-2308745-127.patch59.39 KBg.oechsler
#122 rest-node-dep-2308745-122.interdiff.txt3.85 KBArla
#122 rest-node-dep-2308745-122.patch59.39 KBArla
#115 rest-node-dep-2308745-115.patch56.55 KBArla
#112 interdiff.txt567 bytesAlumei
#112 2308745-rest-node-dep-110.patch57 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2308745-rest-node-dep-110.patch. Unable to apply patch. See the log in the details link for more information. View
#110 2392845-computed-fields-11.no-test.patch6.84 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2392845-computed-fields-11.no-test_0.patch. Unable to apply patch. See the log in the details link for more information. View
#108 2308745-rest-node-dep-108.patch56.45 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,698 pass(es), 2 fail(s), and 1 exception(s). View
#106 2308745-rest-node-dep-106.patch56.27 KBAlumei
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,695 pass(es). View
#106 interdiff.txt14.85 KBAlumei
#102 2308745-rest-node-dep-102.patch47.64 KBAlumei
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,715 pass(es). View
#102 interdiff.txt12.68 KBAlumei
#101 interdiff.txt4.09 KBAlumei
#101 2308745-rest-node-dep-101.patch40.19 KBAlumei
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,765 pass(es). View
#98 interdiff.txt1.88 KBAlumei
#98 2308745-rest-node-dep-98.patch36.55 KBAlumei
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,679 pass(es). View
#94 interdiff.txt1.57 KBAlumei
#94 2308745-rest-node-dep-94.patch36.56 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,010 pass(es), 17 fail(s), and 0 exception(s). View
#90 interdiff.txt3.15 KBAlumei
#90 2308745-rest-node-dep-90.patch37.11 KBAlumei
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,698 pass(es). View
#86 interdiff.txt5.64 KBAlumei
#86 2308745-rest-node-dep-86.patch36.92 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,616 pass(es), 50 fail(s), and 8 exception(s). View
#84 interdiff.txt2.18 KBAlumei
#84 2308745-rest-node-dep-84.patch36.34 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,370 pass(es), 87 fail(s), and 10 exception(s). View
#81 interdiff.txt4.32 KBAlumei
#81 2308745-rest-node-dep-80.patch35.64 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,224 pass(es), 6 fail(s), and 15 exception(s). View
#77 interdiff.txt5.36 KBAlumei
#77 2308745-rest-node-dep-77.patch34.8 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,040 pass(es), 6 fail(s), and 15 exception(s). View
#76 interdiff.txt15.45 KBAlumei
#76 2308745-rest-node-dep-76.patch33.57 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,064 pass(es), 6 fail(s), and 15 exception(s). View
#71 interdiff-31.txt24.17 KBAlumei
#71 interdiff.txt1.98 KBAlumei
#71 2308745-rest-node-dep-71.patch26.39 KBAlumei
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,681 pass(es). View
#70 2308745-rest-node-dep-70.patch25.27 KBAlumei
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,637 pass(es). View
#68 2308745-rest-node-dep-68.patch24.48 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,636 pass(es), 4 fail(s), and 0 exception(s). View
#66 2308745-rest-node-dep-66.patch24.46 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,607 pass(es), 7 fail(s), and 2 exception(s). View
#64 2308745-rest-node-dep-64.patch24.68 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,557 pass(es), 7 fail(s), and 6 exception(s). View
#62 interdiff.txt24.26 KBAlumei
#62 2308745-rest-node-dep-62.patch24.66 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,068 pass(es), 23 fail(s), and 33 exception(s). View
#60 interdiff.txt1.74 KBAlumei
#60 2308745-rest-node-dep-60.patch36.49 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,025 pass(es), 5 fail(s), and 16 exception(s). View
#58 interdiff.txt2 KBAlumei
#58 2308745-rest-node-dep-58.patch36.47 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,091 pass(es), 6 fail(s), and 17 exception(s). View
#56 interdiff.txt825 bytesAlumei
#56 2308745-rest-node-dep-56.patch36.13 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,994 pass(es), 14 fail(s), and 1 exception(s). View
#54 interdiff.txt17.99 KBAlumei
#54 2308745-rest-node-dep-54.patch35.9 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,048 pass(es), 7 fail(s), and 15 exception(s). View
#52 interdiff.txt4.12 KBAlumei
#52 2308745-rest-node-dep-52.patch22.06 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,055 pass(es), 6 fail(s), and 17 exception(s). View
#49 interdiff.txt17.66 KBAlumei
#49 2308745-rest-node-dep-49.patch18.84 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,033 pass(es), 13 fail(s), and 2 exception(s). View
#43 2308745-rest-node-dep-43.patch14 KBAlumei
#31 rest-node-dep-2308745.31.patch5.14 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,367 pass(es). View
#31 interdiff.txt655 byteslarowlan
#24 rest-node-dep-2308745-24.patch5.11 KBR.Muilwijk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,752 pass(es). View
#21 rest-node-dep-2308745.pass_.patch5.13 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,124 pass(es), 6 fail(s), and 3 exception(s). View
#21 rest-node-dep-2308745.fail_.patch3.73 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,873 pass(es), 10 fail(s), and 7 exception(s). View
#13 rest-node-dep-2308745.pass_.patch2.35 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,767 pass(es). View
#13 rest-node-dep-2308745.fail_.patch798 byteslarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,903 pass(es), 1 fail(s), and 1 exception(s). View
#13 interdiff.txt798 byteslarowlan
#8 rest-node-dep-2308745.8.patch1.57 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View
rest-depends-on-node.patch1000 byteslarowlan
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

larowlan’s picture

Priority: Normal » Major

tests/site blow up in spectacular format so figure this is major

Berdir’s picture

This currently serves as documentation I guess, so we should probably move it somewhere?

Also, having a single config file for this is likely going to cause trouble and for example makes it harder to deploy default service configuration with a module, but I doubt we're able to change that now.

clemens.tolboom’s picture

+1 for committing this patch.

Maybe comment the node settings instead to have an in code example?

klausi’s picture

Status: Active » Needs work

We already have docs at https://www.drupal.org/node/2096019 .

I agree with clemens, I think we should comment out the example config since it is surely useful.

larowlan’s picture

Unlike with php, there is a performance hit in large comments in yml.
I think better option would be comment pointing to online documentation.
In reality most people are going to use Rest UI module.

dawehner’s picture

In case this is really helpful, we could just go with an "example." file put in some directory

clemens.tolboom’s picture

Issue summary: View changes

@larowlan how often is the config file scanned? In the past it was copied over to the config/active once.

OTOH using REST UI makes it's content unimportant.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View

@clemens, yeah fair point.

Moved the config to comment.

No interdiff, new patch.

jibran’s picture

Title: Rest module default config depends on node, but rest module does not » REST module default config depends on node, but REST module does not
Berdir’s picture

Yep, comments don't survive an import into active, so it should only affect the initial import, I don't think that makes a measurable difference.

klausi’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Cool, so the only remaining thing to do is a test case that demonstrates the spectacular site blow up that this patch fixes :-)

larowlan’s picture

ok coming right up, cause a test in one of my contrib modules is how I found this :)

larowlan’s picture

Status: Needs work » Needs review
FileSize
798 bytes
798 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,903 pass(es), 1 fail(s), and 1 exception(s). View
2.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,767 pass(es). View
larowlan’s picture

Issue tags: -Needs tests

The last submitted patch, 13: rest-node-dep-2308745.fail_.patch, failed testing.

The last submitted patch, 8: rest-node-dep-2308745.8.patch, failed testing.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Ok, looks good to me!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Adding a web test solely for this purpose seems odd. How about we remove node from the list of modules to enable in RESTTestBase. Which is a doubly good thing since faster tests!

Berdir’s picture

Kernel tests don't install default config, so that will not make it fail unless we'd also force it to import the config.

alexpott’s picture

@Berdir but abstract class RESTTestBase extends WebTestBase

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,873 pass(es), 10 fail(s), and 7 exception(s). View
5.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,124 pass(es), 6 fail(s), and 3 exception(s). View

sure - tries #18, fingers crossed

The last submitted patch, 21: rest-node-dep-2308745.fail_.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: rest-node-dep-2308745.pass_.patch, failed testing.

R.Muilwijk’s picture

FileSize
5.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,752 pass(es). View

Tests failed because RESTTestBase was using self:: in the setUp()

-    // Create a test content type for node testing.
-    $this->drupalCreateContentType(array('name' => 'resttest', 'type' => 'resttest'));
+    // Create a test content type for node testing if appropriate.
+    if (in_array('node', static::$modules)) {
+      $this->drupalCreateContentType(array('name' => 'resttest', 'type' => 'resttest'));
+    }
R.Muilwijk’s picture

Status: Needs work » Needs review
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks proper to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: rest-node-dep-2308745-24.patch, failed testing.

Status: Needs work » Needs review
Arla’s picture

+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -48,15 +48,17 @@
-    // Create a test content type for node testing.
-    $this->drupalCreateContentType(array('name' => 'resttest', 'type' => 'resttest'));
+    // Create a test content type for node testing if appropriate.
+    if (in_array('node', static::$modules)) {
+      $this->drupalCreateContentType(array('name' => 'resttest', 'type' => 'resttest'));
+    }

In the off chance that ATest has 'node' in $modules, and BTest extends ATest, I guess BTest will not have the content type created, but it should? Better change the in_array() check to \Drupal::moduleHandler()->moduleExists('node')?

larowlan’s picture

Issue tags: +Needs reroll
larowlan’s picture

Issue tags: -Needs reroll
FileSize
655 bytes
5.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,367 pass(es). View
larowlan’s picture

back to rtbc?

dawehner’s picture

It is a bit sad, because this makes it harder for people to enable REST support for nodes. It no longer is simple :(

larowlan’s picture

So that's a vote for option 1 - make REST.module depend on node.module?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

No, I think that would be worse, but meh, we seem to have to go with #31

penyaskito’s picture

As berdir mentioned at #2, a single file for providing all entities config causes troubles like this (I faced this in a profile, where I had to set the settings in hook_install() because providing a config file failed.

It could also help with dawehner concerns at #33, as we could provide a rest_node module in core only with the proper config, and it would be simple again.

Is this feasible at this stage?

penyaskito’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think the least disruptive solution is to fix it like this #2397271: REST configuration fails if it contains a plugin reference that does not exist

But the correct solution is to have a configuration entity rest.settings.node so that it is deployable.

larowlan’s picture

So is that a postponed or a duplicate?

alexpott’s picture

@larowlan well the fix in #2397271: REST configuration fails if it contains a plugin reference that does not exist removes the dependency on the node module since with that change wherever rest.settings is used we check that we have a plugin for it. So that's the quick fix.

I think this issue should be repurposed to consider changing the rest module to use config entities. It makes little sense that ...

      $plugin = $this->manager->getInstance(array('id' => $id));

      foreach ($plugin->routes() as $name => $route) {
        $method = $route->getRequirement('_method');
        // Only expose routes where the method is enabled in 

We're checking if the route is enabled in the configuration... the plugin should only return configured routes.

Alumei’s picture

Is the following assumption true:
If rest would use config entities for the endpoint configuration then the node module could provide such a configuration entity as optional configuration?

Arla’s picture

Oh, yes, optional configuration should make this easier. But it seems to me like rest should better ship with that optional configuration for all relevant core modules. (Although possibly "all relevant core modules" really means only "node".)

Alumei’s picture

How about something like this to get started?

Arla’s picture

I don't see any problems with this. The RestEndpointRepository seems like a slightly unnecessary layer on top of the storage, and the actual endpoint config is missing. Also the changes to RestTestBase from the earlier patches should be applied. Let's add it and see some test runs?

clemens.tolboom’s picture

Please fix issue summary as per #40

I think this issue should be repurposed to consider changing the rest module to use config entities.

May I assume we now will #2300677: Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST? If so please close that set of issues preferably with a motivation.

  1. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,49 @@
    \ No newline at end of file
    

    No newline

  2. +++ b/core/modules/rest/src/RestEndpointInterface.php
    @@ -0,0 +1,12 @@
    \ No newline at end of file
    

    No newline

  3. +++ b/core/modules/rest/src/RestEndpointRepository.php
    @@ -0,0 +1,40 @@
    +} ¶
    \ No newline at end of file
    

    Whitespace
    No newline

  4. +++ b/core/modules/rest/src/RestEndpointRepositoryInterface.php
    @@ -0,0 +1,18 @@
    \ No newline at end of file
    

    No newline

Berdir’s picture

No, those two issues have nothing in common. This is about using config entities to store rest.module's configuration.

clemens.tolboom’s picture

Alumei’s picture

Title: REST module default config depends on node, but REST module does not » Solve implicit dependeny of the REST module on node by utilising a config entity type for endpoint configuration
Assigned: Unassigned » Alumei
Issue summary: View changes

Updates issue summary.

I'll work on this now.
I first wanted to get the config entity done first. Now i will work on the feedback and start working down the remaining tasks as state in the IS:

  1. Switch to config entities for REST endpoint configuration
  2. Fix HEAD tests
  3. Incorporate test changes from earlier patches
  4. Use PluginCollections for easy dependency generation and plugin instantiation
  5. Try to move some methods from the Rescource Plugin onto the Configuration entity
Alumei’s picture

Status: Needs work » Needs review
FileSize
18.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,033 pass(es), 13 fail(s), and 2 exception(s). View
17.66 KB

Let's see if HEAD tests work now.

@44: Removed EndpointRepository
@45: Fixed those stupid ones ...

Alumei’s picture

Issue summary: View changes

Added Task to IS:

  • Decide what endpoints should be shipped with the REST module as optional configuration

Node should probably be be only one as the current design would would automatically enable an endpoint if it was shipped as optional configuration if all dependencies are met.
We could also add an active flag to denote whether an endpoint is enabled. In that case it might be better to for each/some core module/s to provide a default configuration instead of the REST module providing one for each of them.

Status: Needs review » Needs work

The last submitted patch, 49: 2308745-rest-node-dep-49.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
22.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,055 pass(es), 6 fail(s), and 17 exception(s). View
4.12 KB

This should fix those test's & also @44: This should contain all relevant changes from #31.

Status: Needs review » Needs work

The last submitted patch, 52: 2308745-rest-node-dep-52.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
35.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,048 pass(es), 7 fail(s), and 15 exception(s). View
17.99 KB

This will hopefully fix the "Invalid character in Config object name" Exceptions.

I changed the the resource plugin ids generated by the EntityDeriver to look like 'entity__*' instead of 'entity:*'.

Status: Needs review » Needs work

The last submitted patch, 54: 2308745-rest-node-dep-54.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
36.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,994 pass(es), 14 fail(s), and 1 exception(s). View
825 bytes

Fixed lines missed in refactoring of RESTTestBase.

Status: Needs review » Needs work

The last submitted patch, 56: 2308745-rest-node-dep-56.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
36.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,091 pass(es), 6 fail(s), and 17 exception(s). View
2 KB

REST endpoint entities now have an correctly working id() ....

Status: Needs review » Needs work

The last submitted patch, 58: 2308745-rest-node-dep-58.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
36.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,025 pass(es), 5 fail(s), and 16 exception(s). View
1.74 KB

This should only have the page cache test's failing.

Status: Needs review » Needs work

The last submitted patch, 60: 2308745-rest-node-dep-60.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
24.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,068 pass(es), 23 fail(s), and 33 exception(s). View
24.26 KB

Apparently neither . nor : are valid as part of a config entity name, switched back to __.
Added optional configuration for the node module as the failing page cache test expects an enabled node rest endpoint after REST module installation.
The plugin id is generated automatically on entity creation.

Status: Needs review » Needs work

The last submitted patch, 62: 2308745-rest-node-dep-62.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
24.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,557 pass(es), 7 fail(s), and 6 exception(s). View

Fixed cache tag assertion in REST module's PageCacheTest.
Fixed permission generation.

Status: Needs review » Needs work

The last submitted patch, 64: 2308745-rest-node-dep-64.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
24.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,607 pass(es), 7 fail(s), and 2 exception(s). View

This should remove the last fatal error. Still no idea why id is not set ...

Status: Needs review » Needs work

The last submitted patch, 66: 2308745-rest-node-dep-66.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
24.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,636 pass(es), 4 fail(s), and 0 exception(s). View

This should fix all fails except the ones in Drupal\page_cache\Tests\PageCacheTest.

Status: Needs review » Needs work

The last submitted patch, 68: 2308745-rest-node-dep-68.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
25.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,637 pass(es). View

This is a long shot but i think that fails because in the REST endpoint for node I hard coded the module dependencies which it should have:

  • node
  • basic_auth
  • hal
Alumei’s picture

Issue summary: View changes
FileSize
26.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,681 pass(es). View
1.98 KB
24.17 KB

Added interdiff against the patch from #31.
This patch also removes the rest.settings.yml and its schema now.
Updated IS to reflect the current progress.

Alumei’s picture

Issue summary: View changes

Updated IS with API changes.

alexpott’s picture

Status: Needs review » Needs work

The

  1. +++ b/core/modules/rest/config/optional/rest.endpoint.entity__node.yml
    @@ -0,0 +1,28 @@
    +dependencies:
    +  module:
    +    - node
    +    - basic_auth
    +    - hal
    

    These dependencies must be calculable and atm I don't see how they are. Also I think that this might belong in node/config/optional

  2. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,85 @@
    +class RestEndpoint extends ConfigEntityBase implements RestEndpointInterface {
    
    +++ b/core/modules/rest/src/RestEndpointInterface.php
    @@ -0,0 +1,22 @@
    +interface RestEndpointInterface extends ConfigEntityInterface {
    

    This should implement EntityWithPluginCollectionInterface

tim.plunkett’s picture

Title: Solve implicit dependeny of the REST module on node by utilising a config entity type for endpoint configuration » Solve implicit dependency of the REST module on node by utilizing a config entity type for endpoint configuration
Alumei’s picture

@73: That's on the todo list ;-)

@73.1: I thought about that as well, whether to put the endpoint into the REST module or node. ++ for node from me

Alumei’s picture

Status: Needs work » Needs review
FileSize
33.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,064 pass(es), 6 fail(s), and 15 exception(s). View
15.45 KB

Added methods to manage the configuration array of the config entity & removed the getter & setter.

Alumei’s picture

FileSize
34.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,040 pass(es), 6 fail(s), and 15 exception(s). View
5.36 KB

The config entity now implements EntityWithPluginCollectionInterface which should calculate the additional dependencies for the resource plugin.

I also wanted to calculate additional dependencies based on the supported formats and authentication provider but those are not identified easily. Apparently both formats and authentication provider are registered via tagged services but there seems to be no way of identifying which module provides that service.
The module could probably be identified by the namespace used for the service class but is there an easier way that i may have overlooked?

The last submitted patch, 76: 2308745-rest-node-dep-76.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 77: 2308745-rest-node-dep-77.patch, failed testing.

Berdir’s picture

Not reviewed yet, just cross-referencing with #2472321: Move rest.module routes to /api, which will conflict a lot I guess.

Alumei’s picture

Status: Needs work » Needs review
FileSize
35.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,224 pass(es), 6 fail(s), and 15 exception(s). View
4.32 KB

Added code to ensure that the request method is upper case and throw an exception if a not matching method is passed.
I could not find an error in the API but maybe there is an error in the usage of the API in tests?

Alumei’s picture

Issue tags: +API change

Realized there is a tag for that ;-)

Status: Needs review » Needs work

The last submitted patch, 81: 2308745-rest-node-dep-80.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
36.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,370 pass(es), 87 fail(s), and 10 exception(s). View
2.18 KB

Well this should fix that ...
Apparently did not see that in protected function enableService($resource_type, $method = 'GET', $format = NULL, $auth = NULL) { the $auth parameter is expected to be an array -_-
Added an array type hint with this patch.

Status: Needs review » Needs work

The last submitted patch, 84: 2308745-rest-node-dep-84.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
36.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,616 pass(es), 50 fail(s), and 8 exception(s). View
5.64 KB

Well this showed that i missed something during refactoring ...
Should be green now.

dawehner’s picture

Just some quick drive by review.

  1. +++ b/core/modules/rest/config/optional/rest.endpoint.entity__node.yml
    @@ -0,0 +1,28 @@
    +plugin_id: "entity:node"
    
    +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,239 @@
    +    // The config entity id looks like the plugin id but uses _ instead of : because : is not valid for config entities.
    +    if (!isset($this->plugin_id) && isset($this->id)) {
    +      $this->plugin_id = str_replace('__', ':', $this->id);
    +    }
    

    Why do you have to care about this here, given that the entity itself seems to have stored that information?

  2. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,239 @@
    +<?php
    +
    

    Nitpick: Missing @file Contains bit.

  3. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,239 @@
    +    $this->pluginManager = \Drupal::service('plugin.manager.rest');
    ...
    +    $plugin_definition = \Drupal::service('plugin.manager.rest')
    +      ->getDefinition(['id' => $this->getResourcePluginID()]);
    

    Do we want to use the "injected" value?

Status: Needs review » Needs work

The last submitted patch, 86: 2308745-rest-node-dep-86.patch, failed testing.

Alumei’s picture

#87.1: That is in there for the first time the entity is created. The plugin id is generated at that time and stored after that. On each subsequent load, the plugin id will be loaded from storage and won't be generated again.

I will fix #87.2 & #87.3 in a subsequent patch hopefully along with the test failures.

Alumei’s picture

FileSize
37.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,698 pass(es). View
3.15 KB

That single missing ! -_-

Alumei’s picture

Status: Needs work » Needs review

For the test bot.

Status: Needs review » Needs work

The last submitted patch, 90: 2308745-rest-node-dep-90.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
36.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,010 pass(es), 17 fail(s), and 0 exception(s). View
1.57 KB

Grasping for straws now ...
Trying to backtrack to find out what is causing those changes.

alexpott’s picture

To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Status: Needs review » Needs work

The last submitted patch, 94: 2308745-rest-node-dep-94.patch, failed testing.

Alumei’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

I tried my best to fill out the template appropriately.

Alumei’s picture

Status: Needs work » Needs review
FileSize
36.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,679 pass(es). View
1.88 KB

How could I ever have expected that the last patch would be green -_-
Maybe this one.

Alumei’s picture

Issue summary: View changes

Updated IS example with the new methods.

Alumei’s picture

FileSize
40.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,765 pass(es). View
4.09 KB
  • Switched to DefaultSingleLazyPluginCollection as we are dealing with only 1 plugin.
  • Updated EntityResource to implement DependentPluginInterface & identify the entity type provider as module dependency.

This should include into the dependencies: the resource plugin provider & in case of an EntityResource derived plugin, the entity type provider

Alumei’s picture

FileSize
12.68 KB
47.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,715 pass(es). View

Renamed <hasSupportedAuthenticationProviders($method) to supportsAuthenticationProviders($method).
Also in lack of any other proposal, implemented my idea from #77:

I also wanted to calculate additional dependencies based on the supported formats and authentication provider but those are not identified easily. Apparently both formats and authentication provider are registered via tagged services but there seems to be no way of identifying which module provides that service.
The module could probably be identified by the namespace used for the service class but is there an easier way that i may have overlooked?

This works as follows:

  • Dependency list generation:
    1. On container rebuild a dependency list for both available authentication providers & formats is generated in RestServiceProvider along the idea of #77
    2. Those lists are saved as a container parameters: rest.dependencies.auth & rest.dependencies.format
  • Dependency calculation for the config entity:
    1. When dependencies for the config entities are calculated in calculateDependencies(), those parameters are accessed via the ContainerAwareTrait
    2. Based on the dependency lists the supported authentication provides and formats for each request method are checked and necessary module dependencies are added.
    3. After this point the config entity should have the appropriate dependencies (At least I could verify that the dependencies looked like the ones on the optional configuration)
  • Fix missing dependencies:
    1. The config entity also implements onDependencyRemoval(array $dependencies)
    2. If a module is uninstalled, the dependency lists are checked as well
    3. If the affected dependencies are related to authentication providers or formats the available request methods are checked for those an the corresponding auth providers and formats are removed from those request methods
    4. Also if the request method has no supported authentication providers or formats, it's removed from the configuration
    5. If there is any request method configuration left afterwards the method will return TRUE marking the dependency problems as resolved
    6. If all dependency problems resolved successfully the config entity will remain in case not, it's deletion won't be prevented(Like for exampe the resource plugin provider was uninstalled)
Alumei’s picture

Issue summary: View changes

I would probably want to cache the affected authentication provider & formats from onDependencyRemoval(array $dependencies) in a static property on the class to improve performance if multiple endpoints are affected.

But before that:

  • Let's see what fails
  • Decide whether to keep this dependency code

Status: Needs review » Needs work

The last submitted patch, 102: 2308745-rest-node-dep-102.patch, failed testing.

Status: Needs work » Needs review
Alumei’s picture

FileSize
14.85 KB
56.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,695 pass(es). View

Tackled "Try to move some methods from the Rescource Plugin onto the Configuration entity".

The patch contains:

  • Made getBaseRoute($canonical_path, $method) public so it can be accessed inside RestEndpoint.
  • Moved route generation from ResourceBase to the config entity type.
  • Now put the REST endpoint id on the route instead of the plugin id.

I only moved the route generation part, because it explicitly depends on configuration.

Once this is green we only have to

  • Decide whether to use the elaborate dependency generation code introduced with #2308745-103: Remove rest.settings.yml, use rest_resource config entities or to rely only on resource plugin dependencies or maybe find another solution
  • Decide what endpoints should be shipped with the REST module as optional configuration or maybe even moved to their respective modules
Alumei’s picture

Assigned: Alumei » Unassigned
Issue summary: View changes

That should be it for now ;-)

Alumei’s picture

FileSize
56.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,698 pass(es), 2 fail(s), and 1 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 108: 2308745-rest-node-dep-108.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
6.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2392845-computed-fields-11.no-test_0.patch. Unable to apply patch. See the log in the details link for more information. View

This should fix the failure by enabling the missing `basic_auth` module. The rest endpoint that this patch ships with the rest module as optional dependency depends on:

  • node
  • basic_auth
  • hal

Status: Needs review » Needs work

The last submitted patch, 110: 2392845-computed-fields-11.no-test.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
57 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2308745-rest-node-dep-110.patch. Unable to apply patch. See the log in the details link for more information. View
567 bytes

Wrong patch .....

Status: Needs review » Needs work

The last submitted patch, 112: 2308745-rest-node-dep-110.patch, failed testing.

Arla’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
56.55 KB

Reroll.

Also, I guess this is unfortunately not welcome in 8.0.x anymore? :)

Status: Needs review » Needs work

The last submitted patch, 115: rest-node-dep-2308745-115.patch, failed testing.

catch’s picture

Yes this could only happen in a minor release.

Also we're going to have to look at a backwards compatibility layer for this somehow, not sure what that looks like though.

catch’s picture

One possible option

- when saving a config entity, convert the old config key to config entities and strip them out of the config object itself.

- when loading a config entity, convert the config entities back to the old config format on the config object

This would cover modules loading and saving the old config object, including default configuration.

When we load, we'd have to add some kind of identifier to indicate that the items are 'fake' to avoid doing the work on save.

Could put this into its own event listener so it's out of the way of everything else, and then potentially could be disabled too. And that event listener is then a place we could trigger E_USER_DEPRECATED in a later release if modules haven't been ported.

If that's actually enough to provide bc, it could be quite a small thing to add.

Crell’s picture

  1. +++ b/core/modules/rest/config/schema/rest.schema.yml
    @@ -45,3 +38,17 @@ rest_request:
    +rest.endpoint.*:
    +  type: config_entity
    +  label: 'REST endpooint'
    

    Pedantic point: There is no such thing as a "rest endpoint". This should be called "rest resource". Endpoints only exist in RPC.

  2. +++ b/core/modules/rest/rest.permissions.yml
    @@ -1,2 +1,5 @@
    +administer rest endpoints:
    +  title: 'Administer REST endpoint configuration'
    

    Ibid.

  3. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,440 @@
    + * Defines a RestEndpoint configuration entity class.
    + *
    + * @ConfigEntityType(
    + *   id = "rest_endpoint",
    

    Ibid. (Will not repeat further.)

  4. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,440 @@
    +  public function __construct(array $values, $entity_type) {
    +    $this->pluginManager = \Drupal::service('plugin.manager.rest');
    +    $this->logger = \Drupal::service('logger.factory')->get('rest');
    +    $this->setContainer(\Drupal::getContainer());
    +    parent::__construct($values, $entity_type);
    

    It's a plugin. Just inject properly with the create() static method. No ContainerAware.

  5. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,440 @@
    +  protected function normaliseRestMethod($method) {
    +    $valid_methods = [ 'GET', 'POST', 'PATCH', 'DELETE' ];
    

    Is there a reason PUT isn't whitelisted? We're not using it for Entities but in concept there's no reason for us to not support it.

While there's a lot of new code here, from an API perspective we're just changing the configuration storage format, no? The API for defining a new REST resource is the same, unless you need dependencies and then it's just an addition. So why do we need a BC layer? I'd think all we need is an upgrade hook to convert the old config file to the new config entities and call it a day.

catch’s picture

@Crell per my previous comment, modules can provide default configuration.

andypost’s picture

@catch modules can only change the config via hook_install() now, because otherwise you will get config import error about the config rest.settings exists (installed with rest module)

That's the second point to get that fixed - provide a way to import rest settings for each resource without affecting a whole config object. The same was done for content_translation.settings and language.settings for scalability & performance reasons already.

So maybe better to re-use third party settings like #2363155: content_translation.settings config is not scalable

EDIT: language settings was changed to config entity #2355909: language.settings config is not scalable

Arla’s picture

Status: Needs work » Needs review
FileSize
59.39 KB
3.85 KB

#119: "I'd think all we need is an upgrade hook to convert the old config file to the new config entities and call it a day."

Did this, and it fixes some fails. Some other fails are caused by what I think is a bug in system_update_8004(), so I'm suggesting a fix there as well. Also suggesting some helpful fail() messages to UpdatePathTestBase.

#121: "modules can only change the config via hook_install() now"

Not sure what you mean by this, @andypost? The update hook seems to work fine. And if we were to use third_party_settings, what entity type would we hang them on to?

Ignored all of the code/naming review points in #119 for now, I want to make the test greener first (and get someone's confirmation on the changes in this patch).

catch’s picture

Status: Needs review » Needs work

"modules can only change the config via hook_install() now"

It means that currently, modules can implement hook_install() to modify the rest configuration object - this is what we need a bc layer for per #118. Similarly with install profile configuration overrides.

The rest update should be numbered 8100 since this will go into 8.1.x

MattA’s picture

Just browsing through the patch file...

  1. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,446 @@
    +  public function __construct(array $values, $entity_type) {
    

    Missing documentation.

  2. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,446 @@
    +    // the dependency lists for authentication providers & formats
    +    //   generated on container build
    ...
    +      // add dependencies based on the supported authentication providers
    ...
    +      // add dependencies based on the supported authentication formats
    

    Punctuation and formatting.

  3. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,446 @@
    +    // Only module related dependencies can be fixed as all other types
    +    //   of dependencies can't as they where not generated based on
    +    //   supported authentication providers or formats
    

    More punctuation and formatting all throughout this method...

  4. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
    @@ -184,23 +130,13 @@ public function availableMethods() {
    -  protected function getBaseRoute($canonical_path, $method) {
    +  public function getBaseRoute($canonical_path, $method) {
    

    Doesn't this break existing plugins that override the method and keep the old visibility?

  5. +++ b/core/modules/rest/src/Plugin/ResourceInterface.php
    @@ -20,18 +21,6 @@
    -  public function routes();
    
    @@ -51,4 +40,16 @@ public function permissions();
    +  public function getBaseRoute($canonical_path, $method);
    

    ...or is it ok to be changing ResourceBase and ResourceInterface?

  6. +++ b/core/modules/rest/src/RestServiceProvider.php
    @@ -0,0 +1,65 @@
    +    // Build a dependency list for available authentication providers
    ...
    +        // extract module name based on class namespace
    ...
    +      // Register the dependency list in the container
    ...
    +    // Build a dependency list for available formats
    ...
    +        // extract module name based on class namespace
    ...
    +      // Register the dependency list in the container
    

    Periods and stuff.

  7. +++ b/core/modules/rest/src/RestServiceProvider.php
    @@ -0,0 +1,65 @@
    +    foreach ($container->findTaggedServiceIds('authentication_provider') AS $service_id => $service_tags) {
    ...
    +    foreach ($container->findTaggedServiceIds('encoder') AS $service_id => $service_tags) {
    ...
    +          foreach ($service_tags AS $service_tag) {
    

    AS => as

AlxVallejo’s picture

Issue tags: +Needs reroll
AlxVallejo’s picture

Status: Needs work » Needs review
g.oechsler’s picture

g.oechsler’s picture

Issue tags: -Needs reroll
g.oechsler’s picture

Now that it applies again let's put some love in it.

It addresses the formatting issues, adds missing doc blocks (#124) and renames the update hook to rest_update_8100 (#123).

dawehner’s picture

  1. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,452 @@
    +    $this->pluginManager = \Drupal::service('plugin.manager.rest');
    +    $this->logger = \Drupal::service('logger.factory')->get('rest');
    +    $this->setContainer(\Drupal::getContainer());
    ...
    +    $auth_dependencies_list = $this->container->getParameter('rest.dependencies.auth');
    +    $format_dependencies_list = $this->container->getParameter('rest.dependencies.format');
    ...
    +    $format_dependencies_list = $this->container->getParameter('rest.dependencies.format');
    +    $auth_dependencies_list = $this->container->getParameter('rest.dependencies.auth');
    

    At least inject the parameters directly instead.

  2. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,452 @@
    +  public function routes() {
    

    IMHO this should stay on the plugin somehow. This kind of method is something people really might want to override on the plugin level. The config entity is just about storage for me, so it should not contain that kind of logic.

  3. +++ b/core/modules/rest/src/Entity/RestEndpoint.php
    @@ -0,0 +1,452 @@
    +  protected function normaliseRestMethod($method) {
    +    $valid_methods = [ 'GET', 'POST', 'PATCH', 'DELETE' ];
    

    nitpick: use remove the space after "["

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -31,7 +35,49 @@
    +    parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $logger);
    +    $this->entity_type_definition = $entity_manager->getDefinition($plugin_definition['entity_type']);
    

    A, let's use $this->entityType and $entity_type_manager instead.

MattA’s picture

Status: Needs review » Needs work

Let's especially not forget to consider 124.4 and 124.5 either.

dawehner’s picture

I'll work a bit on this

Arla’s picture

Status: Needs work » Needs review
FileSize
12.63 KB
59.27 KB

Fixed nitpicks. Big issues remain though:

  • ResourceInterface should not be changed (#124)
  • A config load/save BC layer needs to be added (#118)
  • The "endpoint" entity should be renamed? (#119)

#119.4: EntityInterface::create() ≠ ContainerFactoryPluginInterface::create()... right?

Setting to Needs review to trigger bots although it needs work as per above.

dawehner’s picture

Some changed on top of #133

#119.4: EntityInterface::create() ≠ ContainerFactoryPluginInterface::create()... right?

Yeah :( I'm thinking that it would actually make sense to add this big amount of logic into its own service

Wim Leers’s picture

I was sent the following feedback by Edward Faulkner of Ember.js:

There's no good reason to choose auth methods and serialization formats per endpoint per verb.

I too have been frustrated/surprised by this.

Technically this is off-topic for this issue. But, this issue is completely changing (well, removing) rest.settings.yml. So, if we ever have a chance of changing this, it's in this issue.

Thoughts? An explanation justifying the current design would be welcome too, of course.

dawehner’s picture

Well, we end to support everything what we supported before, otherwise we have an API break, so I don't see how we can change this here.
This though sounds like a discussion for D9. As far as I understand the rest module it tries to be quite flexible in its configuration, so it can fit into more usecases than a normal simple REST integration.

Wim Leers’s picture

Title: Solve implicit dependency of the REST module on node by utilizing a config entity type for endpoint configuration » Remove rest.settings.yml, use rest_endpoint config entities

Changing title to clarify what this issue actually does.

Wim Leers’s picture

Wouldn't rest_resource config entities make more sense than rest_endpoint config entities?

That'd also be consistent with the plugin interface name (https://api.drupal.org/api/drupal/core%21modules%21rest%21src%21Plugin%2...) and the annotation (https://api.drupal.org/api/drupal/core%21modules%21rest%21src%21Annotati...).

dawehner’s picture

Re #135

@Wim Leers
While I understand your points here, we need to take into account two things

a) We want to make progress on this issue itself. It solves a ton of problems rest has at the moment
b) People might have setup crazy things, so let's not try to break things.

IMHO We should split up rest though in two parts:

1. The generic code, usable by probably all REST implementations
2. The concrete rest implementation using the plugin system to register recourses. The second part could then be over time replaced by another module doing something more sane

Wim Leers’s picture

Yep, I understand that, and I agree. I split off #135 into #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources.

My comments #137 and #138 are on-topic/do not make out-of-scope requests, and are just trying to move this issue forward.

neclimdul’s picture

Yeah, this would be pretty nice. Having everything in rest.settings can be a pain in testing as well because you pull along all the other endpoint requirements when testing an individual endpoint. This would allow more targeted, less error prone units of testing for site developers.

Crell’s picture

Switching REST configuration to config entities makes a ton of sense for a myriad of reasons, so +1 on that. However, "endpoint" is the completely wrong terminology here. :-) There is no such thing as a "REST endpoint". REST has resources.

I'm a little unclear from the patch what the scope of the new config entity is. Is it one entity per content entity being exposed (ie, one for node, one for user, one for term, etc.) or one per piece of the content entity being exposed (GET node, POST node, DELETE node, GET user, POST user, DELETE user, etc.)? That will help determine the proper name. (REST Resource is already taken by the main plugin of REST module.)

dawehner’s picture

I'm a little unclear from the patch what the scope of the new config entity is. Is it one entity per content entity being exposed (ie, one for node, one for user, one for term, etc.

The patch uses one per entity type / rest plugin.

Berdir’s picture

So the config entity is the configuration for a rest resource plugin.

Agreed that Resource is a better name for it than Endpoint. Leaves the naming overlap.

A pattern that I quite like for that is a Config suffix. Like FieldConfig for example. Classname would be RestResourceConfig, entity type could be rest_resource or rest_resource_config, and the config prefix either way just rest_resource I think.

Wim Leers’s picture

Title: Remove rest.settings.yml, use rest_endpoint config entities » Remove rest.settings.yml, use rest_resource config entities
Status: Needs review » Needs work

Per #138, #142 and #144.

I don't see why the naming overlap needs to be a problem per se, because one is an annotation, this is a config entity. Confusion seems unlikely. Consistency with FieldConfig is a good argument though.

Crell’s picture

#144 sounds good to me.

Arla’s picture

Status: Needs work » Needs review
FileSize
60.55 KB
22.8 KB

Rename.

Status: Needs review » Needs work

The last submitted patch, 147: rest-2308745-147.patch, failed testing.

Arla’s picture

Removed the test added in #2397271: REST configuration fails if it contains a plugin reference that does not exist. The other fail I cannot find the cause of.

Wim Leers’s picture

+1 to #149. Now the patch removes everything that #2397271: REST configuration fails if it contains a plugin reference that does not exist added.

If you run Drupal\Tests\rest\Kernel\RequestHandlerTest locally, does it pass?

Status: Needs review » Needs work

The last submitted patch, 149: rest-2308745-149.patch, failed testing.

Arla’s picture

No, locally I get the same message "test runner returned a non-zero error code (2) " but cannot find out how to get more information. Nothing in error_log, tried commenting out the two methods one at a time but both give the error.

xjm’s picture

Issue tags: +Needs change record

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Version: 8.2.x-dev » 8.1.x-dev

Patch still applies. I can reproduce the failures locally. Do we still want this in 8.1.x, or do we really move this to 8.2.x? (#154 moved it automatically.)

Moving back to 8.1.x-dev for discussing that.

Wim Leers’s picture

Version: 8.1.x-dev » 8.2.x-dev
17:32:23 <berdir> WimLeers: I don't think 8.1.x is realistic. it is a big change, with a (not yet existing) complicated BC layer that will need a lot of testing

A BC layer that doesn't exist yet is obviously a showstopper.

The last submitted patch, 149: rest-2308745-149.patch, failed testing.

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
61.51 KB

Just a reroll

Status: Needs review » Needs work

The last submitted patch, 159: 2308745-159.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
67.87 KB
6.29 KB

Reroll + bugfix

Status: Needs review » Needs work

The last submitted patch, 161: 2308745-161.patch, failed testing.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
67.88 KB
609 bytes

No comment ....

Status: Needs review » Needs work

The last submitted patch, 163: 2308745-163.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

So much random failure ...

Wim Leers’s picture

Overall, I really really like this patch! I reviewed architecture, not details.

  1. +++ b/core/modules/rest/config/optional/rest.resource.entity__node.yml
    @@ -0,0 +1,28 @@
    +configuration:
    +  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
    

    Per our recent discussion at DrupalCon, which involved many REST stakeholders, I'm wondering if we can't simplify this further:

    configuration:
      formats:
        - hal_json
      auth:
        - basic_auth
      methods:
        - GET
        - POST
        - PATCH
        - DELETE
    

    What do you think?

  2. +++ b/core/modules/rest/rest.install
    @@ -21,3 +23,20 @@ function rest_requirements($phase) {
    +/**
    + * Install the REST config entity type and convert old settings-based config.
    + */
    +function rest_update_8100() {
    +  \Drupal::entityDefinitionUpdateManager()->installEntityType(\Drupal::entityTypeManager()->getDefinition('rest_resource_config'));
    +  foreach (\Drupal::config('rest.settings')->get('resources') as $key => $resource) {
    +    $resource = RestResourceConfig::create([
    +      'id' => str_replace(':', '__', $key),
    +      'configuration' => $resource,
    +    ]);
    +    $resource->save();
    +  }
    +  \Drupal::configFactory()->getEditable('rest.settings')
    +    ->clear('resources')
    +    ->save();
    +}
    

    This is where what I proposed above becomes more complex.

    But… I'd be willing to wager that 99% of sites using REST would actually be able to convert just fine. We could show a warning for the remaining 1% that more things are allowed than before (more formats, more authentication mechanisms).

    Because in upgrading, we'd just take the superset of formats and authentication mechanisms and allow them on all methods.

    IMO that'd be a totally sane upgrade process, with very very minimal disruption.

    Especially if we document it well.

  3. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
    @@ -0,0 +1,380 @@
    +    $this->pluginManager = \Drupal::service('plugin.manager.rest');
    +    $this->logger = \Drupal::service('logger.factory')->get('rest');
    +    $this->setContainer(\Drupal::getContainer());
    

    Can we do better than this, i.e. can we not inject them? If not, let's document why.

  4. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
    @@ -0,0 +1,380 @@
    +  public function calculateDependencies() {
    ...
    +  public function onDependencyRemoval(array $dependencies) {
    

    Hurray!

  5. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
    @@ -0,0 +1,380 @@
    +    $auth_dependencies_list = $this->container->getParameter('rest.dependencies.auth');
    +    $format_dependencies_list = $this->container->getParameter('rest.dependencies.format');
    ...
    +      // Add dependencies based on the supported authentication providers.
    +      foreach ($this->getSupportedAuthenticationProviders($request_method) as $auth) {
    +        if (isset($auth_dependencies_list[$auth])) {
    

    I wonder if this allows for an inconsistent state/race condition?

    I've never seen a calculateDependencies() implementation be tied to data in the container.

    Happy to be wrong though! Just want to be careful wrt dependency metadata, to prevent a state where the user is stuck.

  6. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
    @@ -0,0 +1,380 @@
    +  protected function normaliseRestMethod($method) {
    

    Nit: This is British English, not American English :) And it's inconsistent with Symfony's Serializer::normalize()

  7. +++ b/core/modules/rest/src/Plugin/ResourceInterface.php
    @@ -17,17 +17,6 @@
    -  public function routes();
    
    @@ -46,4 +35,16 @@ public function permissions();
    +  public function getBaseRoute($canonical_path, $method);
    

    Why these API changes?

    AFAICT it's because of the changes in RestResources: it bears the full responsibility for creating routes, rather than the individual resource plugins. So these changes allow @RestResource plugins to actually be declarative: the only non-declarative things they have are the implementations for each of the HTTP methods they support.

    (Those are non-declarative, but also getBaseRoute() implementation they inherit. But that's a good thing: it allows specific REST resources to have more advanced route options, and opt out from the default permissions we also require, or provide a BC layer for existing sites for them, when we land #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources.)

    Is this interpretation correct?

  8. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
    @@ -0,0 +1,130 @@
    +   * @return string The plugin id
    ...
    +   * @return \Drupal\rest\Plugin\ResourceInterface The resource plugin
    ...
    +   * @param string $method The request method
    ...
    +   * @param string $method The request method e.g GET or POST
    +   * @return string[] An array of supported authentication provider plugin id's
    

    Nit: these all belong on a separate line. Occurs in many places.

  9. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
    @@ -0,0 +1,130 @@
    +   * Denotes where a given request method is enabled for this resource.
    

    s/where/whether/

dawehner’s picture

Thanks for this great review! It triggered a couple of changes. I hope these are okay, they have explanations below.

What do you think?

Well sure, the current format is too verbose, but I also disagree with changing this in this issue. We have a dedicated issue for that already: #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources
As we now have an interface changing the internal configuration structure is much easier.

Nit: This is British English, not American English :)

Oh you mean proper english ;)

Can we do better than this, i.e. can we not inject them? If not, let's document why.

Great question. Sadly I cannot find the issue where we tried this last time. It failed horrible. While I tried to inject, what was needed, I realized that actually one should not put too much logic into an entity and extracted the dependency logic into its own service. This also helps to not initialize those services outside of the scope of config dependencies, make the configuration not container aware etc.

I've never seen a calculateDependencies() implementation be tied to data in the container.

I've seen one: #1996130: REST views: not adding dependencies on Serializer providers. I believe that there is no race condition possible: On a full config import, every module installation / uninstallation always fires before other config changes, which then trigger any recalculation of dependencies.
See \Drupal\Core\Config\ConfigImporter::initialize

Why these API changes?

Is this interpretation correct?

The first time I had a look at this patch even more things changed. For example the routes have been entirely defined by the config entities. I think your feeling here is right, we are changing something here we should not change. For me the logic should mostly live in the plugin, as this is what you can override. The config entity itself is just an alternative way to store data, every other change doesn't belong here. This triggered quite some changes all over the place, but I'm quite sure this reduces the patch size quite a bit (spoiler: its just 3 KB less, but it should be easier to review now).

I hope these changes don't cause drama.

Nit: these all belong on a separate line. Occurs in many places.

Fixed that together with some more minor changes

Status: Needs review » Needs work

The last submitted patch, 167: 2308745-166.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
65.22 KB
2.51 KB

I hope that's it.

Wim Leers’s picture

Status: Needs review » Needs work

but I also disagree with changing this in this issue

I knew you were going to say this :) And I actually agree with you!

The main (well, sole) reason that I propose to do it here is because I think doing this sort of change in two steps will be considered too invasive. And because it will mean breaking newly introduced APIs. All of these interface methods are AFAICT tightly coupled to the current configuration structure, because all of them are tied to a particular method:

+++ b/core/modules/rest/src/RestResourceConfigInterface.php
@@ -0,0 +1,165 @@
+  public function isRequestMethodEnabled($method);
...
+  public function getSupportedAuthenticationProviders($method);
...
+  public function hasSupportForAuthenticationProvider($method, $auth);
...
+  public function supportsAuthenticationProviders($method);
...
+  public function addSupportedAuthenticationProvider($method, $auth);
...
+  public function removeSupportedAuthenticationProvider($method, $auth);
...
+  public function getSupportedFormats($method);
...
+  public function supportsFormat($method, $format);
...
+  public function hasSupportedFormats($method);
...
+  public function addSupportedFormat($method, $format);
...
+  public function removeSupportedFormat($method, $format);
As we now have an interface changing the internal configuration structure is much easier.

… it is precisely this interface we would have to change again AFAICT. See the examples of tight coupling I just gave. If you can demonstrate we would not need to change that newly added interface in #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources, then that'd be great, and in that case, ignore what I said in my first point!

I realized that actually one should not put too much logic into an entity and extracted the dependency logic into its own service

This sounds sensible. I see you already did that, yay!

I think your feeling here is right, we are changing something here we should not change.

I'm not sure it's actually better by going back to the way it is in HEAD. But it means fewer changes are introduced. And that I think is a good thing, because it is simply not necessary for this patch to change that, it is not in any way related to how REST resources are configured.
So +1 for reverting to the old logic/behavior/structure/responsibilities.


Interdiff review:

+++ b/core/modules/rest/src/Entity/ConfigDependencies.php
@@ -0,0 +1,157 @@
+class ConfigDependencies {

+++ b/core/modules/rest/src/Entity/RestResourceConfig.php
@@ -259,24 +242,10 @@ public function getPluginCollections() {
+    foreach ($this->getRestResourceDependencies()->calculateDependencies($this) as $type => $dependencies) {

@@ -287,78 +256,25 @@ public function calculateDependencies() {
+    $changed = $this->getRestResourceDependencies()->onDependencyRemoval($this, $dependencies);
...
+   * Returns the REST resource dependencies.

I wonder if ConfigDependencies(Handler|Calculator) or just Dependencies(Handler|Calculator) would be a better name?

Right now when you read the code, especially the code in RestResourceConfig, it looks like you're interacting with dependencies, but you're not, you're only interacting with the logic that calculates/handles dependencies.


  1. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
    @@ -194,8 +194,6 @@ protected function getBaseRoute($canonical_path, $method) {
    -      // Pass the resource plugin ID along as default property.
    -      '_plugin' => $this->pluginId,
    

    These are the only changes in ResourceBase in this patch (yay, less BC break), and I think this change can be reverted?

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -26,7 +30,49 @@
    +class EntityResource extends ResourceBase implements DependentPluginInterface {
    
    @@ -245,4 +291,13 @@ protected function getBaseRoute($canonical_path, $method) {
    +  public function calculateDependencies() {
    

    Is the fact that this plugin now implements this interface actually enough for this plugin's calculateDependencies() to be called and to be taken into account?

  3. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
    @@ -0,0 +1,165 @@
    +  public function getResourcePluginID();
    

    AFAICT this is pointless, because you can already do getResourcePlugin()->getPluginId().

    I guess that's not entirely true, because we also have \Drupal\block\Entity\Block::getPluginId(). But none of the other \Drupal\Core\Entity\EntityWithPluginCollectionInterface classes actually have such a method. So I don't this should either.

    If we want our implementation (RestResourceConfig) to have this, then let's let it implement \Drupal\Component\Plugin\PluginInspectionInterface.

  4. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
    @@ -0,0 +1,165 @@
    +  public function isRequestMethodEnabled($method);
    

    Why not just getSupportedRequestMethods()?

  5. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
    @@ -0,0 +1,165 @@
    +  public function getSupportedAuthenticationProviders($method);
    ...
    +  public function hasSupportForAuthenticationProvider($method, $auth);
    ...
    +  public function supportsAuthenticationProviders($method);
    

    Why these 3? Why not just the first?

  6. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
    @@ -0,0 +1,165 @@
    +  public function addSupportedAuthenticationProvider($method, $auth);
    ...
    +  public function removeSupportedAuthenticationProvider($method, $auth);
    

    These two cause tight coupling to the current configuration system. Also, they're only used in tests. I'd prefer to not have these nice helpers to keep the freedom to change the underlying configuration structure. Tests can just specify the expected array structure instead IMO.

  7. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
    @@ -0,0 +1,165 @@
    +  public function getSupportedFormats($method);
    ...
    +  public function supportsFormat($method, $format);
    ...
    +  public function hasSupportedFormats($method);
    ...
    +  public function addSupportedFormat($method, $format);
    ...
    +  public function removeSupportedFormat($method, $format);
    

    Same story here.

  8. IOW: I'm advocating that we have this:
    interface RestResourceConfigInterface extends ConfigEntityInterface, EntityWithPluginCollectionInterface {
      public function getResourcePlugin();
    
      public function getSupportedRequestMethods();
    
      public function getSupportedAuthenticationProviders($method);
      
      public function getSupportedFormats($method);
    
    }
    

    That: A) removes the (AFAICT) useless "has" and "supports" methods, B) is overall far more compact, C) avoids tight coupling to the current configuration structure, and is therefore forward compatible with #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources.

The last submitted patch, 169: 2308745-169.patch, failed testing.

dawehner’s picture

That: A) removes the (AFAICT) useless "has" and "supports" methods, B) is overall far more compact, C) avoids tight coupling to the current configuration structure, and is therefore forward compatible with #2664780: Remove REST's resource-and-verb-specific permissions or document why it's necessary.

The more I think about it, the more I think we should really just go with it. Let's do that here then. I totally agree that this interface is again simply a c&p of every possible method rather than a designed interface. Thank you wim for insisting on it!
@Wim Leers Do you want to give it a try?

These are the only changes in ResourceBase in this patch (yay, less BC break), and I think this change can be reverted?

Well yeah, the question is whether we want to keep it a property which is not used anymore, but sure we can keep it.

AFAICT this is pointless, because you can already do getResourcePlugin()->getPluginId().

Agreed!

Is the fact that this plugin now implements this interface actually enough for this plugin's calculateDependencies() to be called and to be taken into account?

What else could the entity plugin depend on?

Wim Leers’s picture

Thank you wim for insisting on it! @Wim Leers Do you want to give it a try?

:)
If I do so, I can't RTBC this patch anymore. So I'll let you do so, if you don't mind.

Well yeah, the question is whether we want to keep it a property which is not used anymore, but sure we can keep it.

Oh, right, never mind that!

What else could the entity plugin depend on?

Sorry, I explained myself poorly! I didn't mean that it would depend on something else. I meant that I didn't understand how this calculateDependencies() method would ever be called, how its results would make its way back into the ResourceConfigEntity.

dawehner’s picture

Status: Needs work » Needs review
FileSize
65.64 KB
5.23 KB

So yeah, if we really want to change the support configuration we should first discuss that with other people. For example I'm not sure whether requiring the less verbose configuration is possible in D8 itself.

If I do so, I can't RTBC this patch anymore. So I'll let you do so, if you don't mind.

You know, there are other problems too have, so yeah I personally wait until we actually know what we want to do.

We can simplify your configuration even more by skipping the top level 'configuration key':

  formats:
    - hal_json
  auth:
    - basic_auth
  methods:
    - GET
    - POST
    - PATCH
    - DELETE

Status: Needs review » Needs work

The last submitted patch, 174: 2308745-174.patch, failed testing.

Wim Leers’s picture

Assigned: Unassigned » klausi
+++ b/core/modules/rest/src/Entity/ConfigDependencies.php
@@ -67,6 +68,11 @@ public function calculateDependencies(RestResourceConfigInterface $rest_config)
+    if (($resource_plugin = $rest_config->getResourcePlugin()) && $resource_plugin instanceof DependentPluginInterface) {
+      $dependencies = array_merge($dependencies, $resource_plugin->calculateDependencies());
+    }

Aha, so this part was missing!

Now I understand how this works :) Thanks!

We can simplify your configuration even more by skipping the top level 'configuration key':

+1!

if we really want to change the support configuration we should first discuss that with other people.

Agreed. Assigning to @klausi for feedback. And pinged @catch and @alexpott for feedback also.

alexpott’s picture

We can't skip the top level configuration key because the top level has to be a mapping and not a sequence :( There are issues out there discussing this... eg. #2248709: The root type of a configuration object can not be a sequence

dawehner’s picture

We can't skip the top level configuration key because the top level has to be a mapping and not a sequence :( There are issues out there discussing this... eg. #2248709: The root type of a configuration object can not be a sequence

Well this is status quo, but with the simplified configuration syntax you have just 'formats', 'auth' and 'methods' there, which are all sequences.

dawehner’s picture

Status: Needs work » Needs review
FileSize
65.69 KB
2.67 KB

This just fixes some test failures.

From the discussion with alexpott yesterday it seemed to be that we cannot just drop the verbosity of the old configuration.

Wim Leers’s picture

From the discussion with alexpott yesterday it seemed to be that we cannot just drop the verbosity of the old configuration.

Well…

18:35:37 <dawehner> alexpott: what is the policy around removing features in 8.x?
18:36:16 <alexpott> dawehner: depends - I guess you're thinking about the rest.settings format?
18:36:24 <dawehner> alexpott: exactly
18:37:46 <alexpott> dawehner: I think changing the config structure might be okay if we've got an upgrade path
18:38:16 <alexpott> dawehner: the problem is - combinations that were previously configurable no longer are.
18:38:31 <dawehner> alexpott: well after that people might expose more formats / auths for some methods
18:38:37 <dawehner> alexpott: exactly, well this is my question
18:38:42 <alexpott> mpdonadio: btw - you might be right - maybe there is a way the prefix can clash
18:38:45 <dawehner> alexpott: do we want to magically expose more than before
18:39:13 <alexpott> dawehner: I think this is tricky
18:39:34 <alexpott> dawehner: is there no way to support two formats of configuration?
18:39:48 <dawehner> alexpott: conceptually totally
18:40:14 <dawehner> alexpott: I mean especially now that we have an interface we can totally hide a simplified and more advanced configuration format behind it
18:40:35 <alexpott> dawehner: so that's what I think we're going to have to do
18:40:49 <alexpott> dawehner: unfortunately
18:40:55 <dawehner> alexpott: yeah I mean we totally promised a BC policy
18:41:04 <dawehner> noone said that its going to painless
18:47:37 <WimLeers> alexpott: catch suggested that we let the existing rest.settings.yml continue to exist (necessary for BC anyway), and we convert it upon importing
18:47:59 <WimLeers> which I quite like conceptually
18:48:39 <alexpott> WimLeers: I think that is okay - kinda why I was hoping that the config entity conversion might be done at the same time
18:48:52 <alexpott> WimLeers: or are we not going down the config entity route anymore
…
19:15:08 <WimLeers> alexpott: +1 for doing it at the same time for that very reason
19:15:18 <WimLeers> alexpott: dawehner was concerned it'd increase the scope too much
19:15:43 <WimLeers> alexpott: but that's exactly what I think we need to do: we need to convert to config entity AND do the simplification in one issue, otherwise we risk painting ourselves in a corner
19:15:56 <alexpott> WimLeers: I can see why - but changing everything only to change it again means we end up supporting three formats
19:16:02 <WimLeers> alexpott: exactly!
19:16:18 <dawehner> alexpott: WimLeers I can totally see the points you make here, just trying to let us not get crazy
19:16:33 <WimLeers> dawehner: oh yes, absolutely!
19:16:40 <alexpott> dawehner, WimLeers: sorry I have to go
19:16:42 <WimLeers> dawehner: I'm not talking bad about you, just presenting the discussion :)
19:16:45 <dawehner> alexpott: have a good one!
19:16:54 <WimLeers> alexpott: np, this was helpful! Good night!
19:16:55 <dawehner> WimLeers: oh yeah also just saying where I'm coming from
19:16:59 <WimLeers> dawehner: yep, absolutely
19:17:05 <dawehner> WimLeers: it would be great to understand why we have this verbose format in the first place
19:17:16 <WimLeers> dawehner: let me do some archeology for that
19:17:47 <dawehner> WimLeers: I also asked klausi
19:18:38 <WimLeers> dawehner: I did too — see #drupal-wscci
19:18:46 <WimLeers> dawehner: I think he'll comment, but will need some time
19:18:54 <WimLeers> dawehner: I can do the archeology in the mean time

I started doing the archeology, but in doing so I ended up finding the full answer to #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources's problems, so I wrote that up in #2664780-11: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources and managed to finish that patch mostly. Now finishing up my archeological digging…

11:32:17 <alexpott> dawehner: wrt to rest.settings and BC I don't think we have consensus yet
11:32:36 <dawehner> alexpott: that is true, IMHO we should start once we are done with KTBTNG
11:33:30 <dawehner> alexpott: for me keeping 'configuration' for now and then in an additional issue add the formats bit and generate the more verbose configuration internally sounds the less problematic solution
11:36:59 <alexpott> dawehner: but what happens when we move to config entities?
11:37:27 <dawehner> alexpott: well I assume we start with moving to config entities first

We already talked about exactly that, see the earlier chat transcript. I agree we don't have consensus yet, but moving to config entities first doesn't help, because then we end up with three formats we have to support:

  1. rest.settings as it exists today
  2. a straight migration of the verbosity/specificity of rest.settings into rest_resource config entities
  3. and then a simplification of those rest_resource config entities

That's the part that is untenable as far as I can tell.

That is why I wrote what I wrote in #166.1 and #166.2.

Wim Leers’s picture

I did some archeology, to find out why exactly we ended up with the current rest.settings design, which configured supported formats and supported authenticated mechanisms per method per resource, instead of only per resource, or even configuring those for all resources.

So, much like the way the permission handling in REST was done with barely any critical review, as I described at #2664780-11: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources, the same is true here. Times have changed; this would never have been able to land in the past few years.

dawehner’s picture

Thank you for your dirty diving. Ancient information are there.
In general no question, in general people tend to nick without thinking, when there are too many changes at the same time, which is actually a contra point to any major short time change. Just small evolutionary steps can really provide a good end result.

We already talked about exactly that, see the earlier chat transcript. I agree we don't have consensus yet, but moving to config entities first doesn't help, because then we end up with three formats we have to support:
rest.settings as it exists today
a straight migration of the verbosity/specificity of rest.settings into rest_resource config entities
and then a simplification of those rest_resource config entities

Are we sure we cannot remove support for the old format, as we provide a clear 1to1 update path?

In general I doubt there is too much overhead connected with providing a BC layer, but well, let's first simplify the interface, so at least supporting that might be actually easier.

If @alexpott kinda thinks about a BC layer, and well, let's be honest, he is one of the more liberal core committers, I cannot imagine how other would react to it.

Status: Needs review » Needs work

The last submitted patch, 182: 2308745-182.patch, failed testing.

Wim Leers’s picture

Are we sure we cannot remove support for the old format, as we provide a clear 1to1 update path?

Oh!!! You're saying it's 100% guaranteed we can provide a complete upgrade path from rest.settings's resources key to rest_resource config entities? That's totally true, that's what this patch basically does.

So then we end up with two types of config entities, or two versions/levels. I almost wonder if it makes sense to call the config entity we're introducing here the rest_resource_verbose config entities? And then the simplified ones could be called rest_resource?

In general I doubt there is too much overhead connected with providing a BC layer, but well, let's first simplify the interface, so at least supporting that might be actually easier.

+100, I was hoping you'd do that :)


    This review basically reviews the #182 interdiff.

  1. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
    @@ -0,0 +1,118 @@
    +  public function getResourcePluginID();
    

    In #172, you agreed to also remove this one.

  2. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
    @@ -0,0 +1,118 @@
    +  public function addSupportedAuthenticationProvider($method, $auth);
    ...
    +  public function removeSupportedAuthenticationProvider($method, $auth);
    ...
    +  public function addSupportedFormat($method, $format);
    ...
    +  public function removeSupportedFormat($method, $format);
    

    I'm still concerned that these are here, I think these may still tie the interface too closely to the current configuration structure (everything per-method).

dawehner’s picture

Status: Needs work » Needs review
FileSize
63.68 KB

In #172, you agreed to also remove this one.

No idea how this sneaked in again.

So then we end up with two types of config entities, or two versions/levels. I almost wonder if it makes sense to call the config entity we're introducing here the rest_resource_verbose config entities? And then the simplified ones could be called rest_resource?

That could be a great solution to the problem. The interface would then just have methods for the non verbose case maybe?

Just rerolled the patch due to recent great improvements!

Status: Needs review » Needs work

The last submitted patch, 185: 2308745-185.patch, failed testing.

Wim Leers’s picture

That could be a great solution to the problem.

Glad you like the idea.

What do klausi & Alex Pott think?

The interface would then just have methods for the non verbose case maybe?

Exactly!

klausi’s picture

Can we support specifying the format on the resource level and on the method level at the same time in one config entity? We could throw an exception if someone mixes those 2 variants and specifies the format on both levels. Or not, then the method level format would override the resource level format. Same for authentication.

Wim Leers’s picture

#138: I don't see how we can do that with config schema.

Wim Leers’s picture

FYI: I'm trying to prove myself wrong: I'm trying to make this happen with config schema. I think it may actually be possible.

Wim Leers’s picture

HURRAY! I was able to prove myself wrong :) :)

I found a way to make it possible to:

  1. continue to support the current syntax, which is very, very granular: configure format/auth per method per resource. The upgrade path is trivial: add a top-level type: granular.
  2. also support the simpler syntax we'd like to see: the one @dawehner listed in #174. This syntax can be used by specifying type: simple.

Attached is a patch that updates REST's config schema to support this. And another patch that provides an example rest.settings.yml to use it. Apply both, and verify using https://www.drupal.org/project/config_inspector that it is indeed working.

I am not saying we must add support for this simple variant here, although I personally think that may actually be better. But, if we agree that this is what we want rest_resource config entities do use by default (the type: simple) variant, then this can inform what the RestResourceConfigInterface must look like for both types to be supported.


Finally, for completeness, the relevant IRC transcript from last Friday, where this was discussed among @klausi, @dawehner and I, where we were in full agreement to proceed with this approach:

16:51:55 <WimLeers> klausi: I'm playing with config schema to try to prove myself (my reply to your comment) wrong
16:52:16 <klausi> WimLeers: yeah, don't try too hard, was just an idea
16:52:30 <WimLeers> klausi: the more I think about it it, the more I like it
16:52:56 <WimLeers> klausi: this is what it'd look like. BC would be trivial, it'd just be adding a "type: granular" key to all of the existing resources resources:
  entity:node:
    type: granular
    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
  entity:user:
    type: simple
    formats:
      - hal_json
    auth:
      - basic_auth
    methods:
      - GET
      - POST
      - PATCH
      - DELETE
16:53:42 <klausi> not bad!
16:54:58 <WimLeers> klausi: you'd support that?
16:55:11 <klausi> WimLeers: sure
16:55:32 <WimLeers> klausi: alright, let me continue to find a way :P
16:55:54 <WimLeers> klausi: I struggled for a while to make the D8 CDN module's config schema do what I wanted
16:55:59 <WimLeers> klausi: I'm hopeful I can do the same here
16:56:07 <WimLeers> dawehner: ^^
16:56:09 <klausi> oh yes, we all know the config schema pains
16:56:50 <dawehner> WimLeers: nice, could we even default to type: simple somehow?
16:56:58 <WimLeers> dawehner: I *think* so, yes!
16:57:06 <dawehner> WimLeers: that would be perfect
16:57:15 <WimLeers> dawehner: I'd ask Gábor, but he's driving somewhere atm
16:57:19 <WimLeers> dawehner: I totally agree
16:57:26 <WimLeers> dawehner: klausi okay, so we have very strong consensus now
16:57:35 <WimLeers> this would completely solve BC concerns
16:57:47 <WimLeers> now we just need to make it work :P
17:00:02 <dawehner> WimLeers: are you sure you can default to it?
17:00:09 <WimLeers> I'm not sure at all
17:00:11 <WimLeers> I'm hopeful :P
17:00:15 <WimLeers> I vaguely recall something like that
17:00:22 <dawehner> WimLeers: i mean config schema doesn't do any logic beside whats in the file
17:00:23 <WimLeers> I fought with exactly that sort of thing for the CDN module's config schema
17:00:28 <WimLeers> indeed
17:01:16 <WimLeers> dawehner: though even if 'type: simple' is not optional, that would not be a big deal imo
17:09:31 <dawehner> WimLeers: agreed
17:09:49 <WimLeers> cool :)
17:09:53 <WimLeers> #exciting
17:11:34 <WimLeers> dawehner++
17:11:36 <WimLeers> klausi++
17:11:38 <WimLeers> klausi++
17:11:40 <WimLeers> dawehner++
MattA’s picture

I like where this is headed, but I have a quick suggestion. Can we make the type names a bit more descriptive and/or related to the config property's purpose?

Ex:
type: granular --> type: per_method
type: simple --> type: per_resource

Wim Leers’s picture

#192: ooh, perhaps granularity: resource and granularity: method? There's no need to call the key type, it can be anything.

Wim Leers’s picture

Assigned: klausi » Wim Leers

Talked to @dawehner in IRC.

We're now swapping roles. He's going to review, I'm going to reroll, since he's not going to have the bandwidth to reroll this patch.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
64.44 KB
595 bytes

#2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it landed and caused a conflict, and also required a change in UpdateTest, because as of #2631774, node isn't being installed by default anymore in RESTTestBase.

So, this is still a straight reroll. This doesn't fix any of the test failures. Doing that next.

Wim Leers’s picture

One tiny thing was overlooked in #182, that's what the first hunk of this interdiff fixes. That's what causes all of the failures here, except for those in PageCacheTest. Those I still need to fix.

The rest in this patch is to address a small bug that has existed for longer in this patch, but is only exposed/causing failing tests since #2626298: REST module must cache only responses to GET requests, which was committed after #182/#185. Otherwise, there would be many more failures.

So, this should bring it down to far fewer failures.

Wim Leers’s picture

+++ b/core/modules/rest/src/Tests/PageCacheTest.php
@@ -84,17 +84,17 @@ public function testConfigChangePageCache() {
-    $this->httpRequest($url, 'GET', NULL, $this->defaultMimeType);
...
+    $this->httpRequest($entity->urlInfo()->setRouteParameter('_format', $this->defaultFormat), 'GET', NULL, $this->defaultMimeType);

This unnecessary change is what causing the fails in PageCacheTest: the $entity object here does not have an ID, hence no URL can be generated.

The last submitted patch, 195: rest_settings_config_entity-2308745-195.patch, failed testing.

The last submitted patch, 196: rest_settings_config_entity-2308745-196.patch, failed testing.

Wim Leers’s picture

Given the broad agreement for #188–#193, to have two granularities with which you can configure REST resources, which allows us to keep BC easily, I'm addressing that first. That's the biggest remaining obstacle.

This patch then introduces granularity: method, but does NOT introduce granularity: resource. That's for #2721595: Simplify REST configuration to do, but #191's attachments contain everything that is necessary to be able to do that, config-schema-wise.

Wim Leers’s picture

Alright, we now have a green patch again, that has a solution for how to deal with first doing this, and then simplifying the configuration (in #2721595: Simplify REST configuration). It'd be great to hear from @dawehner, @klausi and others whether they're still +1.
Note that a consequence of this is that in several places, we'll need forked logic: "if granularity is method, do X, if granularity is resource, do Y". However, that is of course the cost of BC, and that's something we cannot avoid.


Next step: look into simplifying the interface further, like I said in #184.2.

Also, #200 made me notice we don't yet have upgrade path tests.

dawehner’s picture

Alright, we now have a green patch again, that has a solution for how to deal with first doing this, and then simplifying the configuration (in #2721595: [PP-1] Simplify REST configuration). It'd be great to hear from @dawehner, @klausi and others whether they're still +1.

+1

Wim Leers’s picture

#202: yay!


Small step forward in simplifying RestResourceConfig(Interface): per #87.3 and #170.3, we can get rid of getResourcePluginID().

MattA’s picture

  1. +++ b/core/modules/rest/rest.install
    @@ -21,3 +23,22 @@ function rest_requirements($phase) {
    + * @todo Automatically upgrade those REST resource config entities that have the same formats/auth mechanisms for all methods to "granular: resource" in https://www.drupal.org/node/2721595.
    

    80-char limit

  2. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
    @@ -0,0 +1,189 @@
    +  protected function calculateDependenciesForMethodGranularity(RestResourceConfigInterface $rest_config) {
    

    Optional: This could probably be shortened into calculateDependenciesByMethod()?

  3. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
    @@ -0,0 +1,189 @@
    +   *   An array of dependencies that will be deleted keyed by dependency type.
    

    Could probably use a comma after "deleted."

  4. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
    @@ -0,0 +1,189 @@
    +        // Only mark the dependencies problems as fixed if there is any
    +        // configuration left.
    

    Might have been looking for the possessive form of dependency instead of the plural form. Rephrase?

  5. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
    @@ -0,0 +1,267 @@
    +  public function getSupportedAuthenticationProviders($method) {
    ...
    +  public function addSupportedAuthenticationProvider($method, $auth) {
    ...
    +  public function removeSupportedAuthenticationProvider($method, $auth) {
    ...
    +  public function getSupportedFormats($method) {
    ...
    +  public function addSupportedFormat($method, $format) {
    ...
    +  public function removeSupportedFormat($method, $format) {
    

    Is "supported" really necessary in these method names? I might have glossed over some stuff, but I'm not seeing a use-case for getting all the installed providers/formats at the config entity level.

dawehner’s picture

Is "supported" really necessary in these method names? I might have glossed over some stuff, but I'm not seeing a use-case for getting all the installed providers/formats at the config entity level.

Good point!

Wim Leers’s picture

Per myself in #170.8:

IOW: I'm advocating that we have this:

interface RestResourceConfigInterface extends ConfigEntityInterface, EntityWithPluginCollectionInterface {
  public function getResourcePlugin();

  public function getSupportedRequestMethods();

  public function getSupportedAuthenticationProviders($method);
  
  public function getSupportedFormats($method);

}

That: A) removes the (AFAICT) useless "has" and "supports" methods, B) is overall far more compact, C) avoids tight coupling to the current configuration structure, and is therefore forward compatible with #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources.

and @dawhener's confirmation of that in #171:

That: A) removes the (AFAICT) useless "has" and "supports" methods, B) is overall far more compact, C) avoids tight coupling to the current configuration structure, and is therefore forward compatible with #2664780: Remove REST's resource-and-verb-specific permissions or document why it's necessary.

The more I think about it, the more I think we should really just go with it. Let's do that here then. I totally agree that this interface is again simply a c&p of every possible method rather than a designed interface. Thank you wim for insisting on it!
@Wim Leers Do you want to give it a try?

I started doing this. Each of these points has a corresponding interdiff. Consider each a commit. This makes reviewing easier, but avoids me posting a gazillion separate patches.

  1. I added getMethods() (I know I said getSupportedRequestMethods(), but that seems overly verbose and obvious). I removed isRequestMethodEnabled(). I already made it easy for #2721595 to add its code for resource-level granularity — also because that makes it more clear what the end result will look like. Which is important for determining how maintainable the end result will be.
    16 LoC less, and simpler interface.
  2. Upon doing so, the $this->get('configuration')['granularity'] quickly got annoying. So I moved it outside of configuration, into the top level.
  3. Renamed getSupportedAuthenticationProviders() to getAuthenticationProviders(), removed addSupportedAuthenticationProvider() and removeSupportedAuthenticationProvider().
    40 LoC less, and simpler interface.
  4. Renamed getSupportedFormats() to getFormats(), removed addSupportedFormat() and removeSupportedFormat().
    54 LoC less, and simpler interface.

Related to interdiff 2: @dawehner already alluded to this in #178. But there, he wanted to go further: the resource-level granularity doesn't need the top-level 'configuration' key, he says. This is true. But if we remove that key, then we can no longer choose either method- or resource-level granularity within the same config entity.
However… we totally can if we make GET, POST etc into a mapping rather than a sequence. Every key in a mapping is optional anyway. (Everything is optional in config.)


#204.1–4: Thanks for the nits! Will address later.

#204.5 + #205: hah, that's exactly what I already did :)


Next step: roll a sister patch for #2721595: Simplify REST configuration, to prove that this can indeed work.

Wim Leers’s picture

Before rolling that sister patch I mentioned at the end of #206, it's more practical to first have upgrade test coverage, because that other issue will need its own upgrade path test, relative to this one.

So, here's upgrade path test coverage.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
68.94 KB
3.41 KB

While I started working on the sister patch, I noticed many more places weren't yet making the granularity-based distinction that they will need to. This introduces that distinction in all necessary places.

This also clearly visualized the cost of this approach: every RestResourceConfigInterface method implementation forks into two distinct implementations: one for either granularity. The same goes for the calculateDependencies() and onDependencyRemoval() methods: we need two distinct implementations, one for each granularity.

I think this is the true cost of BC. This is why I originally argued for removing this capability of configuring up to a per-method granularity: is it really a BC break to suddenly have the union/superset of all formats and authentication mechanisms be available for all methods on a resource? I'm not convinced it is.


I'll now stop working on this patch. Only remaining todos: address nitpicks, including those in #204.1–4.

I'm now working on a sister patch for #2721595: Simplify REST configuration that builds on top of this one, and hence provides a picture of what the overall end result would look like, if we continue to support both configuration granularities.

Wim Leers’s picture

And voila, granularity: resource is now implemented at #2721595-7: Simplify REST configuration, with a patch that includes upgrade path test coverage, and with an actual testable patch that stacks on top of patch #208 here. It should come back green.

That leaves this patch at a point where it's more feasible than before to properly evaluate the consequences.

IMO this is now blocked on review from at least:

  1. klausi
  2. dawehner
  3. Alex Pott
dawehner’s picture

I think this is the true cost of BC. This is why I originally argued for removing this capability of configuring up to a per-method granularity: is it really a BC break to suddenly have the union/superset of all formats and authentication mechanisms be available for all methods on a resource? I'm not convinced it is.

Well, it all comes down to our definition of BC. When we define just our formats as the officially supported ones I would agree with your point. On the other hand there might be crazy custom formats which exposes some data you should normally not see on a GET request. I would rather be defensive than exposing anything to the world. There is a reason we support BC, and well of course we need a price to pay for it. There is also a reason why D9 will be released at some point.

This also clearly visualized the cost of this approach: every RestResourceConfigInterface method implementation forks into two distinct implementations: one for either granularity. The same goes for the calculateDependencies() and onDependencyRemoval() methods: we need two distinct implementations, one for each granularity.

Yeah entity interfaces are a lie anyway. There will always be just one implementation of it. You know as long the complexity is just within the entity, this seems totally reasonable for me. We added an abstraction which makes it possible to implement some custom behaviour underneath it, which is some form of success, at least for me. When we are afraid of the if statements you could implement a plugin type for the granularity but I'm really not sure its worth the effort.

  1. +++ b/core/modules/rest/src/RestResourceConfigInterface.php
    @@ -0,0 +1,51 @@
    +
    +  /**
    +   * Retrieves the REST resource plugin.
    +   *
    +   * @return \Drupal\rest\Plugin\ResourceInterface
    +   *   The resource plugin
    +   */
    +  public function getResourcePlugin();
    +
    +  /**
    +   * Retrieves a list of supported HTTP methods.
    +   *
    +   * @return string[]
    +   *   A list of supported HTTP methods.
    +   */
    +  public function getMethods();
    +
    +  /**
    +   * Retrieves a list of supported authentication providers.
    +   *
    +   * @param string $method
    +   *   The request method e.g GET or POST.
    +   *
    +   * @return string[]
    +   *   A list of supported authentication provider IDs.
    +   */
    +  public function getAuthenticationProviders($method);
    +
    +  /**
    +   * Retrieves a list of supported response formats.
    +   *
    +   * @param string $method
    +   *   The request method e.g GET or POST.
    +   *
    +   * @return string[]
    +   *   A list of supported format IDs.
    +   */
    +  public function getFormats($method);
    +
    

    Nice interface!

  2. +++ b/core/modules/rest/src/RestServiceProvider.php
    @@ -0,0 +1,60 @@
    +   */
    +  public function register(ContainerBuilder $container) {
    

    Its conceptually better to put that information into a compiler pass, as we derive information from existing registered classes. Note: \Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass and \Drupal\serialization\RegisterSerializationClassesCompilerPass are both implemented as path.

  3. +++ b/core/modules/rest/src/RestServiceProvider.php
    @@ -0,0 +1,60 @@
    +    foreach ($container->findTaggedServiceIds('authentication_provider') as $service_id => $service_tags) {
    +      $service_def = $container->findDefinition($service_id);
    +      $service_class = $service_def->getClass();
    +      if (!empty($service_class)) {
    +        // Extract module name based on class namespace.
    +        list($namespace_base, $module_name) = explode('\\', $service_class);
    +        if (!empty($namespace_base) && ucfirst(strtolower($namespace_base)) == 'Drupal') {
    +          // Remove the 'authentication.' prefix from the provider ID.
    +          $provider_id = substr($service_id, 15);
    +          $auth[$provider_id] = $module_name;
    +        }
    +      }
    +    }
    +    if (!empty($auth)) {
    +      // Register the dependency list in the container.
    

    For me authentication is something provided by core itself, so this data should be added by core. Authentication is orthogonal to rest. #2228141: Add authentication support to REST views would like to use it as well.

  4. +++ b/core/modules/rest/src/RestServiceProvider.php
    @@ -0,0 +1,60 @@
    +      }
    +    }
    +    if (!empty($formats)) {
    +      // Register the dependency list in the container.
    +      $container->setParameter('rest.dependencies.format', $formats);
    +    }
    +  }
    

    Formats are conceptually not an idea of the rest module but rather an idea of the serialization module. Also, isn't rest.dependencies.format exactly the same as serializer.format_providers at the moment?

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

There is a reason we support BC, and well of course we need a price to pay for it. There is also a reason why D9 will be released at some point.

Agreed.

On the other hand there might be crazy custom formats which exposes some data you should normally not see on a GET request. I would rather be defensive than exposing anything to the world.

Fair! And we should definitely not forget that. I forgot that for a moment there. But you're totally right, and that's the reason we're willing to do this.

You know as long the complexity is just within the entity, this seems totally reasonable for me.

Agreed!

We added an abstraction which makes it possible to implement some custom behaviour underneath it, which is some form of success, at least for me. When we are afraid of the if statements you could implement a plugin type for the granularity but I'm really not sure its worth the effort.

Agreed :)


  • #210.1: :)
  • #210.2–4: hah, I forgot to write that in my last comment, but I agree, that's the last thing in this patch that feels… wonky. I had the exact same reservations! Let me already address those.
Wim Leers’s picture

Addressed #210.2–4.

Wim Leers’s picture

The last submitted patch, 212: rest_settings_config_entity-2308745-212.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 213: rest_settings_config_entity-2308745-213.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
75.49 KB
679 bytes

Hah, this uncovered another nice bug: \Drupal\Tests\rest\Kernel\RestLinkManagerTest doesn't install serialization module, but it should.

This will be green again.

dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record
FileSize
75.57 KB
0 bytes
1.05 KB

I think this looks really great. Just fixing two small nitpicks. Also wrote some change record in the meantime.

Wim Leers’s picture

Great, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/rest/config/schema/rest.schema.yml
    @@ -45,3 +40,20 @@ rest_request:
    +      type: rest_resource.[%parent.granularity]
    
    +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
    @@ -0,0 +1,215 @@
    +      // 'granularity' is not a request method
    +      if ($request_method === 'granularity') {
    +        continue;
    +      }
    

    This seems weird to have on the same level as methods then. Why do we have that? I guess config schema parsing - it's not necessary we can use something like %parent.granularity... hmmm that's what you've done... is this necessary?

  2. +++ b/core/modules/rest/rest.install
    @@ -21,3 +23,33 @@ function rest_requirements($phase) {
    +      'id' => str_replace(':', '__', $key),
    
    +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
    @@ -0,0 +1,267 @@
    +    // The config entity id looks like the plugin id but uses __ instead of :
    +    // because : is not valid for config entities.
    +    if (!isset($this->plugin_id) && isset($this->id)) {
    +      // Generate plugin_id on first entity creation.
    +      $this->plugin_id = str_replace('__', ':', $this->id);
    +    }
    

    I think this should be a . to be more like how fields handles this.

  3. +++ b/core/modules/rest/rest.services.yml
    @@ -24,9 +24,12 @@ services:
    +    arguments: ['%serializer.format_providers%', '%authentication_providers%']
    

    Why can't we inject the authentication_collector here and not create the authentication_provider container param?

  4. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
    @@ -0,0 +1,215 @@
    +    if (($resource_plugin = $rest_config->getResourcePlugin()) && $resource_plugin instanceof DependentPluginInterface) {
    +      $dependencies = array_merge($dependencies, $resource_plugin->calculateDependencies());
    +    }
    

    This looks necessary for both levels - looks like it could move to ConfigDependencies::calculateDependencies(). Actually thinking about this maybe we want to implement EntityWithPluginCollectionInterface on the config entity. Since this gives as a lot for free. @timplunkett is the expert on these. But doing this should make this code redundant AND keep plugin and config in sync. See ConfigEntityBase::set().

  5. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
    @@ -0,0 +1,215 @@
    +      foreach ($dependencies['module'] as $dep_module) {
    +        // Check if the removed dependency module contained an authentication
    +        // provider.
    +        foreach ($this->authProviders as $auth => $auth_module) {
    +          if ($dep_module != $auth_module) {
    +            continue;
    +          }
    +          $removed_auth[] = $auth;
    +        }
    +        // Check if the removed dependency module contained a format.
    +        foreach ($this->formatProviders as $format => $format_module) {
    +          if ($dep_module != $format_module) {
    +            continue;
    +          }
    +          $removed_formats[] = $format;
    +        }
    +      }
    

    I think this can be reduced to two lines using array_intersect - it preserves keys... so:

    $removed_auth = array_keys(array_interest($this->authProviders, $dependencies['module']));
    $removed_formats = array_keys(array_intersect($this->formatProviders, $dependencies['module']));
    
  6. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
    @@ -0,0 +1,215 @@
    +          foreach ($removed_formats as $format) {
    +            if (in_array($format, $rest_config->getFormats($request_method))) {
    +              $rest_config = array_filter($rest_config[$request_method]['supported_formats'], function ($val) use ($format) {
    +                return ($val != $format);
    +              });
    +            }
    +          }
    

    This code looks odd because it's setting $rest_config which is the config entity - and using array access... not sure whats going on here.

  7. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
    @@ -0,0 +1,215 @@
    +          foreach ($removed_auth as $auth) {
    +            if (in_array($auth, $rest_config->getAuthenticationProviders($request_method))) {
    +              $rest_config = array_filter($rest_config[$request_method]['supported_auth'], function ($val) use ($auth) {
    +                return ($val != $auth);
    +              });
    +            }
    +          }
    

    As above

  8. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
    @@ -0,0 +1,267 @@
    + *   label = @Translation("REST resource config"),
    

    I don't think we should be using the word config. Maybe just Rest resource or change config to configuration.

  9. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
    @@ -0,0 +1,267 @@
    +  protected $methods = [];
    +  protected $auth = [];
    +  protected $formats = [];
    

    Need documenting.

  10. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
    @@ -0,0 +1,267 @@
    +      // @todo Add resource-level granularity support in https://www.drupal.org/node/2721595.
    ...
    +      // @todo Add resource-level granularity support in https://www.drupal.org/node/2721595.
    ...
    +      // @todo Add resource-level granularity support in https://www.drupal.org/node/2721595.
    

    I think we should throw an exception on the unsupported code path for now.

  11. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
    @@ -0,0 +1,267 @@
    +    return $this->dependencies;
    

    return $this; - this changed in Drupal 8 beta. Yes it is tricky because plugin's version does something different - sorry.

  12. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
    @@ -265,7 +265,15 @@ protected function runUpdates() {
    -    $this->assertFalse(\Drupal::service('entity.definition_update_manager')->needsUpdates(), 'After all updates ran, entity schema is up to date.');
    +    $needs_updates = \Drupal::entityDefinitionUpdateManager()->needsUpdates();
    +    $this->assertFalse($needs_updates, 'After all updates ran, entity schema is up to date.');
    +    if ($needs_updates) {
    +      foreach (\Drupal::entityDefinitionUpdateManager()->getChangeSummary() as $entity_type_id => $summary) {
    +        foreach ($summary as $message) {
    +          $this->fail($message);
    +        }
    +      }
    +    }
    

    Should have a separate issue - looks extremely sensible.

  13. The dependencies stuff and especially onDependencyRemoval() needs tests
Wim Leers’s picture

  1. That's a left-over from an earlier iteration, that can indeed be removed!
  2. Ok, we'll do that.
  3. We could do that, but @dawhener pointed out that A) this is symmetrical then with how we list all available serialization formats as a container parameter, B) he'll need exactly the same elsewhere: in #2228141: Add authentication support to REST views. See his comment at #210.3.
  4. Note that the other granularity is not being implemented here — it'll be introduced at #2721595: Simplify REST configuration. So I think this belongs in the other issue?
  5. Ok, we'll do that.
  6. Probably @dawehner can improve this. You're saying this should not use array access?
  7. Probably @dawehner can improve this.
  8. +1
  9. +1
  10. Sure.
  11. Ok, nice catch.
  12. @dawehner, you want to take that on?
  13. YES! You're totally right, in doing the refactoring, we ended up forgetting about that. @dawehner, since you wrote the dependency handling code some time ago and know best how that stuff works, do you want to take that on?
alexpott’s picture

Re point 4 well using EntityWithPluginCollectionInterface is nothing to do with granularity.

alexpott’s picture

I also think that this issue highlights the need for #2628144: Ensure that ConfigImport is taking place against the same code base to prevent people re-importing configuration in the old rest.settings config.

[Edit: fixed the issue link]

dawehner’s picture

Status: Needs work » Needs review
FileSize
81.96 KB
31.38 KB

This seems weird to have on the same level as methods then. Why do we have that? I guess config schema parsing - it's not necessary we can use something like %parent.granularity... hmmm that's what you've done... is this necessary?

Good point, yeah this is indeed no longer necessary.

Why can't we inject the authentication_collector here and not create the authentication_provider container param?

Well, having the actual instances of the providers is nice, but they don't really want us to tell anything about their modules providing them, even if you ask really friendly.

This looks necessary for both levels - looks like it could move to ConfigDependencies::calculateDependencies(). Actually thinking about this maybe we want to implement EntityWithPluginCollectionInterface on the config entity. Since this gives as a lot for free. @timplunkett is the expert on these. But doing this should make this code redundant AND keep plugin and config in sync. See ConfigEntityBase::set().

OH yeah, that might indeed help quite a bit, I'll explore that.

array_interest

Its certainly in the interest of array to have some kind of union.

I don't think we should be using the word config. Maybe just Rest resource or change config to configuration.

I think it helps to distinct the rest resource plugins from the actual configurations.

Need documenting.

Well, actually they need removal. All three aren't used.

I think we should throw an exception on the unsupported code path for now.

Good idea!

YES! You're totally right, in doing the refactoring, we ended up forgetting about that. @dawehner, since you wrote the dependency handling code some time ago and know best how that stuff works, do you want to take that on?

Oh yeah totally, let's see

dawehner’s picture

alexpott’s picture

Oops... array_interest() = array_intersect()

dawehner’s picture

I also think that this issue highlights the need for #2308745: Remove rest.settings.yml, use rest_resource config entities to prevent people re-importing configuration in the old rest.settings config.

Which issue do you actually talk about here?

Status: Needs review » Needs work

The last submitted patch, 223: 2308745-222.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
81.92 KB
623 bytes

We already implement that interface ...

Status: Needs review » Needs work

The last submitted patch, 228: 2308745-228.patch, failed testing.

alexpott’s picture

+++ b/core/modules/rest/src/Entity/ConfigDependencies.php
@@ -0,0 +1,191 @@
+    if (($resource_plugin = $rest_config->getResourcePlugin()) && $resource_plugin instanceof DependentPluginInterface) {
+      $dependencies = NestedArray::mergeDeep($dependencies, $resource_plugin->calculateDependencies());
+    }

If you already implement the interface - I think this code is unnecessary.

dawehner’s picture

Status: Needs work » Needs review
FileSize
81.91 KB
4.68 KB

Let's see, this could fix a couple of the issues.

Status: Needs review » Needs work

The last submitted patch, 231: 2308745-231.patch, failed testing.

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review

.

Wim Leers’s picture

Looks great! This addressed all of @alexpott's remarks AFAICT.

  1. @@ -1,4 +1,4 @@
    -id: entity__node
    +id: entity.node
    

    This makes so much sense! I'm happy @alexpott asked for that.

  2. +++ b/core/modules/rest/rest.install
    @@ -5,7 +5,7 @@
    -use Drupal\rest\Entity\RestResourceConfig;
    +use Drupal\rest\Entity\RestResourceConfiguration;
    

    Why this rename?

    16:15:39 <WimLeers> dawehner: currently reviewing your REST configuration patch
    16:16:18 <WimLeers> dawehner: the one thing I don't yet understand is why you renamed not just the label but all of the code from RestResourceConfig(Interface) to RestResourceConfiguration(Interface)
    16:16:30 <WimLeers> Now it's inconsistent with FieldStorageConfig
    16:17:00 <dawehner> WimLeers: mh right, most classes actually use Config
    16:17:01 <dawehner> like Config
    16:17:06 <WimLeers> y
    
  3. +++ b/core/modules/rest/src/Entity/ConfigDependencies.php
    @@ -138,57 +134,36 @@ public function onDependencyRemoval(RestResourceConfigInterface $rest_config, ar
    -  public function onDependencyRemovalForMethodGranularity(RestResourceConfigInterface $rest_config, array $dependencies) {
    +  protected function onDependencyRemovalForMethodGranularity(RestResourceConfigurationInterface $rest_config, array $dependencies) {
    

    No longer public: totally makes sense.

  4. +++ b/core/modules/rest/src/Entity/RestResourceConfiguration.php
    @@ -4,15 +4,16 @@
    + *   label = @Translation("REST resource configuration"),
    

    I think just renaming this would've been good enough — I think that's all that @alexpott was asking.

alexpott’s picture

#230 is still to be addressed...

+++ b/core/modules/rest/src/Entity/ConfigDependencies.php
@@ -0,0 +1,191 @@
+    if (($resource_plugin = $rest_config->getResourcePlugin()) && $resource_plugin instanceof DependentPluginInterface) {
+      $dependencies = NestedArray::mergeDeep($dependencies, $resource_plugin->calculateDependencies());
+    }

Not needed (i think)

Wim Leers’s picture

Status: Needs review » Needs work

For #235 + #236.

dawehner’s picture

Status: Needs work » Needs review
FileSize
82.1 KB
19.96 KB

Yeah let's revert those changes.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#238 addressed #235 + #236. No more remarks.

Hurray!

dawehner’s picture

Thank you!

alexpott’s picture

Patched attached replaces the hard coding everywhere of 'method' with a constant because it's better. And fixes a few coding standards issues. Leaving at RTBC because I don't think these changes warrant going back to needs review - but I wanted a test run before committing and a +1 from @Wim Leers or @dawehner.

alexpott’s picture

Discussed with @dawehner in IRC and we discussed whether the constant should be on RestResourceConfig or RestResourceConfigInterface. At first it seemed that moving to the concrete class was correct because it felt an implementation detail of RestResourceConfig but it is also used extensively by the service this patch adds so I think the interface is better.

alexpott’s picture

+++ b/core/modules/rest/rest.services.yml
@@ -24,9 +24,12 @@ services:
+  rest_resource.dependencies:
+    class: \Drupal\rest\Entity\ConfigDependencies
+    arguments: ['%serializer.format_providers%', '%authentication_providers%']

Discussed this with @dawehner - it's not really a service - he considered it a private service but as we're using \Drupal::service() it can't be private. We decided to remove it from the container and use the class resolver to inject its dependencies. The results in a separation of concerns whilst not making it a swappable service.

dawehner’s picture

Works for me. Thank you!

alexpott’s picture

+++ b/core/modules/rest/rest.install
@@ -21,3 +24,33 @@ function rest_requirements($phase) {
+function rest_update_8201() {
+  \Drupal::entityDefinitionUpdateManager()->installEntityType(\Drupal::entityTypeManager()->getDefinition('rest_resource_config'));
+  foreach (\Drupal::config('rest.settings')->get('resources') as $key => $resource) {
+    $resource = RestResourceConfig::create([
+      'id' => str_replace(':', '.', $key),
+      'granularity' => RestResourceConfigInterface::METHOD_GRANULARITY,
+      'configuration' => $resource,
+    ]);
+    $resource->save();
+  }
+  \Drupal::configFactory()->getEditable('rest.settings')
+    ->clear('resources')
+    ->save();
+}

Are we sure that using the config entity API is alright in a hook_update_N? I'm not sure. I think we probably should do a little dance. Ie. I think we need to fix rest.settings here but then create the config entities in a post update. So we set it in state or something and go from there?

dawehner’s picture

Well, in this case its just create ... in general though I don't see a reason this could not be post update. There is nothing to repair.

Wim Leers’s picture

+1 for everything said here.

alexpott’s picture

So discussed a bit more with @dawehner - we agreed that we shouldn't be using the full config entity API in hook_update_N(). So in the hook_update_N() I save the current resources to state... \Drupal::state()->set('rest_update_8201_resources', \Drupal::config('rest.settings')->get('resources'));. At the moment the patch leaves this in state. I think this is good idea - just in case we need to do something with this information cause we missed something with the post update.

Wim Leers’s picture

Looks great.

  • alexpott committed 871da5e on 8.2.x
    Issue #2308745 by Alumei, dawehner, Wim Leers, larowlan, Arla, alexpott...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.2.0 release notes

Committed 871da5e and pushed to 8.2.x. Thanks!

Wim Leers’s picture

Whew! OMG! Yes!

So glad this is in. A few months ago, I thought it'd be impossible :)

This unblocked #2721595: Simplify REST configuration.

The IS also mentioned this:

Tasks after the comment:

Did that too (changes: https://www.drupal.org/node/2667736/revisions/view/9764057/9794157), and updated the CR for that too. Also clarified the CR to explain why this was necessary.

webflo’s picture

I tried the update path from 8.1.3 to 8.2.x via drush. I got the Failed: The "rest_resource_config" entity type does not exist. on the first try. I think we have to rebuild the entity type registry? The update worked flawless on the second drush updb. I did not call drush cr before the first drush updb. Worth a follow-up?

dawehner’s picture

Thank you for your testing @webflo

You are just so right!

+ \Drupal::entityDefinitionUpdateManager()->installEntityType(\Drupal::entityTypeManager()->getDefinition('rest_resource_config'));

This line is the problematic line. Conceptually we should NOT get the definition from the entity type manager, but rather hardcode the definition in the update function which was available at the point in time we wrote the code.
So we better fix the update function I guess. Practically it doesn't matter than much because this is a config entity anyway, which doesn't deal with all that.

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

skyredwang’s picture

I am trying to test D8.2.2 with Drupal Commerce 2-beta3.

REST UI recognizes commerce_order's endpoint as /admin/commerce/orders/{commerce_order}. I couldn't get this endpoint working neither /commerce_order/{commerce_order}, or /entity/commerce_order/{commerce_order}

On the other hand, I was trying to declare endpoints, using code. But, I am not sure what this paragraph mean exactly?

Each REST resource has a \Drupal\rest\RestResourceConfigInterface config entity that corresponds to a @RestResource plugin. Without such a config entity, the REST resource plugin will not be available for use.

Does this mean each module has to write a little code to support REST?

By the way, I also tried /entity/rest_resource_config/{rest_resource_config}, I see WSOD.

Can someone write a short tutorial to get a not-too-easy entity working? (Not node, taxonomy, etc)

R.Muilwijk’s picture