We've accumulated a lot of BC layers in the rest.module component. In this issue, we maintain an up-to-date patch that removes all BC layers, which allows us to track how much of a difference this would make.

Note that for this to land, we need #2893795: Remove serialization.module BC layers to land first.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Title: Remove serialization.module BC layers » Remove rest.module BC layers
FileSize
105.23 KB

This removes all BC layers from core/modules/rest as of July 11, 2017, commit e1e216138f5311a5dd6b96b5789ab2db9ac5fd5f.

(Note that this must be applied on top of #2893795-2: Remove serialization.module BC layers.)

 core/modules/rest/config/install/rest.settings.yml |   7 -
 core/modules/rest/config/schema/rest.schema.yml    |  14 -
 core/modules/rest/rest.api.php                     |  57 --
 core/modules/rest/rest.install                     |  60 --
 core/modules/rest/rest.post_update.php             |  63 ---
 core/modules/rest/rest.routing.yml                 |   9 -
 core/modules/rest/rest.services.yml                |  15 -
 .../src/EventSubscriber/RestConfigSubscriber.php   |  53 --
 .../ConfigurableLinkManagerInterface.php           |  13 -
 core/modules/rest/src/LinkManager/LinkManager.php  |  13 -
 .../rest/src/LinkManager/LinkManagerBase.php       |  13 -
 .../rest/src/LinkManager/LinkManagerInterface.php  |  13 -
 .../rest/src/LinkManager/RelationLinkManager.php   |  13 -
 .../LinkManager/RelationLinkManagerInterface.php   |  13 -
 .../rest/src/LinkManager/TypeLinkManager.php       |  13 -
 .../src/LinkManager/TypeLinkManagerInterface.php   |  13 -
 .../PathProcessorEntityResourceBC.php              |  55 --
 core/modules/rest/src/Plugin/ResourceBase.php      |   8 -
 .../src/Plugin/rest/resource/EntityResource.php    |  10 +-
 core/modules/rest/src/RestServiceProvider.php      |  51 --
 core/modules/rest/src/Tests/RESTTestBase.php       | 624 ---------------------
 .../Update/EntityResourcePermissionsUpdateTest.php |  56 --
 .../Tests/Update/ResourceGranularityUpdateTest.php |  71 ---
 .../Update/RestConfigurationEntitiesUpdateTest.php |  65 ---
 .../src/Tests/Update/RestExportAuthUpdateTest.php  |  36 --
 ....rest-rest_post_update_resource_granularity.php | Bin 5199 -> 0 bytes
 .../update/drupal-8.rest-rest_update_8201.php      |  59 --
 .../update/drupal-8.rest-rest_update_8203.php      |  58 --
 .../update/rest-export-with-authentication.php     |  65 ---
 .../rest.resource.entity.comment_2721595.yml       |  32 --
 .../update/rest.resource.entity.node_2721595.yml   |  29 -
 .../update/rest.resource.entity.user_2721595.yml   |  32 --
 .../BcTimestampNormalizerUnixTestTrait.php         |  43 --
 .../BaseFieldOverrideResourceTestBase.php          |   4 -
 .../EntityResource/Block/BlockResourceTestBase.php |   4 -
 .../Comment/CommentResourceTestBase.php            |  17 +-
 .../Editor/EditorResourceTestBase.php              |   4 -
 .../EntityResource/EntityResourceTestBase.php      | 171 ------
 .../EntityTest/EntityTestResourceTestBase.php      |  12 +-
 .../EntityTestLabelResourceTestBase.php            |  12 +-
 .../EntityResource/Feed/FeedResourceTestBase.php   |  22 +-
 .../FieldConfig/FieldConfigResourceTestBase.php    |   4 -
 .../FieldStorageConfigResourceTestBase.php         |   4 -
 .../ImageStyle/ImageStyleResourceTestBase.php      |   4 -
 .../EntityResource/Item/ItemResourceTestBase.php   |  12 +-
 .../MenuLinkContentResourceTestBase.php            |  12 +-
 .../EntityResource/Node/NodeResourceTestBase.php   |  22 +-
 .../NodeType/NodeTypeResourceTestBase.php          |   4 -
 .../SearchPage/SearchPageResourceTestBase.php      |   4 -
 .../Shortcut/ShortcutResourceTestBase.php          |   4 -
 .../EntityResource/Term/TermResourceTestBase.php   |  12 +-
 .../EntityResource/Tour/TourResourceTestBase.php   |   4 -
 .../EntityResource/User/UserResourceTestBase.php   |  17 +-
 53 files changed, 62 insertions(+), 1963 deletions(-)
Wim Leers’s picture

Status: Active » Postponed

(This module likely has the most BC layers and the most BC layer test coverage of all of core I think, considering how much we can remove…)

Wim Leers’s picture

FileSize
103.08 KB

Rebased for 8.5.x, commit 0fa72bdeea3d72e874b25dc72d2185645b57d6c5.

Wim Leers’s picture

Related issues: +#2720233: Use arguments resolver in RequestHandler to have consistent parameter ordering
FileSize
2.63 KB
105.65 KB

Updating for BC layers added to #2720233: Use arguments resolver in RequestHandler to have consistent parameter ordering.

Slightly bigger diffstat now:

 core/modules/rest/config/install/rest.settings.yml |   7 -
 core/modules/rest/config/schema/rest.schema.yml    |  14 -
 core/modules/rest/rest.api.php                     |  57 --
 core/modules/rest/rest.install                     |  60 --
 core/modules/rest/rest.post_update.php             |  63 ---
 core/modules/rest/rest.routing.yml                 |   9 -
 core/modules/rest/rest.services.yml                |  15 -
 .../src/EventSubscriber/RestConfigSubscriber.php   |  53 --
 .../ConfigurableLinkManagerInterface.php           |  13 -
 core/modules/rest/src/LinkManager/LinkManager.php  |  13 -
 .../rest/src/LinkManager/LinkManagerBase.php       |  13 -
 .../rest/src/LinkManager/LinkManagerInterface.php  |  13 -
 .../rest/src/LinkManager/RelationLinkManager.php   |  13 -
 .../LinkManager/RelationLinkManagerInterface.php   |  13 -
 .../rest/src/LinkManager/TypeLinkManager.php       |  13 -
 .../src/LinkManager/TypeLinkManagerInterface.php   |  13 -
 .../PathProcessorEntityResourceBC.php              |  55 --
 core/modules/rest/src/Plugin/ResourceBase.php      |   8 -
 .../src/Plugin/rest/resource/EntityResource.php    |  10 +-
 core/modules/rest/src/RequestHandler.php           |  41 +-
 core/modules/rest/src/RestServiceProvider.php      |  51 --
 core/modules/rest/src/Tests/RESTTestBase.php       | 624 ---------------------
 ....rest-rest_post_update_resource_granularity.php | Bin 5199 -> 0 bytes
 .../update/drupal-8.rest-rest_update_8201.php      |  59 --
 .../update/drupal-8.rest-rest_update_8203.php      |  58 --
 .../update/rest-export-with-authentication.php     |  65 ---
 .../rest.resource.entity.comment_2721595.yml       |  32 --
 .../update/rest.resource.entity.node_2721595.yml   |  29 -
 .../update/rest.resource.entity.user_2721595.yml   |  32 --
 .../BcTimestampNormalizerUnixTestTrait.php         |  43 --
 .../BaseFieldOverrideResourceTestBase.php          |   4 -
 .../EntityResource/Block/BlockResourceTestBase.php |   4 -
 .../Comment/CommentResourceTestBase.php            |  17 +-
 .../Editor/EditorResourceTestBase.php              |   4 -
 .../EntityResource/EntityResourceTestBase.php      | 171 ------
 .../EntityTest/EntityTestResourceTestBase.php      |  12 +-
 .../EntityTestLabelResourceTestBase.php            |  12 +-
 .../EntityResource/Feed/FeedResourceTestBase.php   |  22 +-
 .../FieldConfig/FieldConfigResourceTestBase.php    |   4 -
 .../FieldStorageConfigResourceTestBase.php         |   4 -
 .../ImageStyle/ImageStyleResourceTestBase.php      |   4 -
 .../EntityResource/Item/ItemResourceTestBase.php   |  12 +-
 .../MenuLinkContentResourceTestBase.php            |  12 +-
 .../EntityResource/Node/NodeResourceTestBase.php   |  22 +-
 .../NodeType/NodeTypeResourceTestBase.php          |   4 -
 .../SearchPage/SearchPageResourceTestBase.php      |   4 -
 .../Shortcut/ShortcutResourceTestBase.php          |   4 -
 .../EntityResource/Term/TermResourceTestBase.php   |  12 +-
 .../EntityResource/Tour/TourResourceTestBase.php   |   4 -
 .../EntityResource/User/UserResourceTestBase.php   |  17 +-
 .../Update/EntityResourcePermissionsUpdateTest.php |  56 --
 .../Update/ResourceGranularityUpdateTest.php       |  71 ---
 .../Update/RestConfigurationEntitiesUpdateTest.php |  65 ---
 .../Functional/Update/RestExportAuthUpdateTest.php |  36 --
 54 files changed, 63 insertions(+), 2003 deletions(-)
Wim Leers’s picture

FileSize
7.97 KB
113.32 KB

Updating for BC layers added in #2825204: REST views: authentication is broken. Which added a whole bunch of BC layer/update path code that can be removed. ~180 additional line deletions:

 core/modules/rest/config/install/rest.settings.yml |   7 -
 core/modules/rest/config/schema/rest.schema.yml    |  14 -
 core/modules/rest/rest.api.php                     |  57 --
 core/modules/rest/rest.install                     |  96 ----
 core/modules/rest/rest.module                      |  27 -
 core/modules/rest/rest.post_update.php             |  63 ---
 core/modules/rest/rest.routing.yml                 |   9 -
 core/modules/rest/rest.services.yml                |  15 -
 .../src/EventSubscriber/RestConfigSubscriber.php   |  53 --
 .../ConfigurableLinkManagerInterface.php           |  13 -
 core/modules/rest/src/LinkManager/LinkManager.php  |  13 -
 .../rest/src/LinkManager/LinkManagerBase.php       |  13 -
 .../rest/src/LinkManager/LinkManagerInterface.php  |  13 -
 .../rest/src/LinkManager/RelationLinkManager.php   |  13 -
 .../LinkManager/RelationLinkManagerInterface.php   |  13 -
 .../rest/src/LinkManager/TypeLinkManager.php       |  13 -
 .../src/LinkManager/TypeLinkManagerInterface.php   |  13 -
 .../PathProcessorEntityResourceBC.php              |  55 --
 core/modules/rest/src/Plugin/ResourceBase.php      |   8 -
 .../src/Plugin/rest/resource/EntityResource.php    |  10 +-
 .../rest/src/Plugin/views/display/RestExport.php   |  18 -
 core/modules/rest/src/RequestHandler.php           |  41 +-
 core/modules/rest/src/RestServiceProvider.php      |  51 --
 core/modules/rest/src/Tests/RESTTestBase.php       | 626 ---------------------
 .../Update/RestExportAuthCorrectionUpdateTest.php  |  36 --
 ....rest-rest_post_update_resource_granularity.php | Bin 5199 -> 0 bytes
 .../update/drupal-8.rest-rest_update_8201.php      |  59 --
 .../update/drupal-8.rest-rest_update_8203.php      |  58 --
 .../rest-export-with-authentication-correction.php |  63 ---
 .../update/rest-export-with-authentication.php     |  65 ---
 .../rest.resource.entity.comment_2721595.yml       |  32 --
 .../update/rest.resource.entity.node_2721595.yml   |  29 -
 .../update/rest.resource.entity.user_2721595.yml   |  32 --
 .../BcTimestampNormalizerUnixTestTrait.php         |  43 --
 .../BaseFieldOverrideResourceTestBase.php          |   4 -
 .../EntityResource/Block/BlockResourceTestBase.php |   4 -
 .../Comment/CommentResourceTestBase.php            |  17 +-
 .../Editor/EditorResourceTestBase.php              |   4 -
 .../EntityResource/EntityResourceTestBase.php      | 174 ------
 .../EntityTest/EntityTestResourceTestBase.php      |  12 +-
 .../EntityTestLabelResourceTestBase.php            |  12 +-
 .../EntityResource/Feed/FeedResourceTestBase.php   |  22 +-
 .../FieldConfig/FieldConfigResourceTestBase.php    |   4 -
 .../FieldStorageConfigResourceTestBase.php         |   4 -
 .../ImageStyle/ImageStyleResourceTestBase.php      |   4 -
 .../EntityResource/Item/ItemResourceTestBase.php   |  12 +-
 .../MenuLinkContentResourceTestBase.php            |  12 +-
 .../EntityResource/Node/NodeResourceTestBase.php   |  22 +-
 .../NodeType/NodeTypeResourceTestBase.php          |   4 -
 .../SearchPage/SearchPageResourceTestBase.php      |   4 -
 .../Shortcut/ShortcutResourceTestBase.php          |   4 -
 .../EntityResource/Term/TermResourceTestBase.php   |  12 +-
 .../EntityResource/Tour/TourResourceTestBase.php   |   4 -
 .../EntityResource/User/UserResourceTestBase.php   |  17 +-
 .../Update/EntityResourcePermissionsUpdateTest.php |  56 --
 .../Update/ResourceGranularityUpdateTest.php       |  71 ---
 .../Update/RestConfigurationEntitiesUpdateTest.php |  65 ---
 .../Functional/Update/RestExportAuthUpdateTest.php |  36 --
 58 files changed, 63 insertions(+), 2188 deletions(-)
Wim Leers’s picture

FileSize
115.63 KB

There have been quite a few REST changes lately. Patch didn't apply anymore.

Rebased on top of commit 91e668f5758159b27bea9e4bc3247969fa91232a, solved all conflicts. Because of removed whitespace, there are now fewer deletions:

58 files changed, 63 insertions(+), 2168 deletions(-)

Also: I should've been queueing tests for this, to prove that it works. Did that this time.

Wim Leers’s picture

More REST tests have been added, I haven't updated them all perfectly, hence tests not being able to run.

5 more files changed:

 63 files changed, 79 insertions(+), 2182 deletions(-)
Wim Leers’s picture

This should fix some of the failures.

Wim Leers’s picture

#2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage added some extra code to ensure BC layers are respected by the support for caching responses:

+      // Add global rest settings config's cache tag, for BC flags.
+      // @see \Drupal\rest\Plugin\rest\resource\EntityResource::permissions()
+      // @see \Drupal\rest\EventSubscriber\RestConfigSubscriber
+      // @todo Remove in https://www.drupal.org/node/2893804
+      $response->addCacheableDependency($this->configFactory->get('rest.settings'));

(and more)

That can now all be removed too.

Diffstat keeps getting more appealing:

 68 files changed, 92 insertions(+), 2275 deletions(-)
Wim Leers’s picture

FileSize
132.02 KB
error: cannot apply binary patch to 'core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php' without full index line
error: core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php: patch does not apply

I hate git diff --binary with a passion. A) git is stupid for thinking it's binary, B) why can't I configure git to always create binary patches!?

This patch is identical to 11, but with --binary.

Wim Leers’s picture

Drupal\dblog\Tests\Rest\DbLogResourceTest fails with a fatal error, because it uses a deprecated base class that this patch is removing. Created #2927758: Update DbLogResourceTest to use the ResourceTestBase base class instead of the deprecated RESTTestBase to update that test to no longer use the deprecated base test class. That now blocks this patch from becoming green.

Wim Leers’s picture

Title: Remove rest.module BC layers » [PP-3] Remove rest.module BC layers

This is blocked on #13 + #14 + #15.

Wim Leers’s picture

FileSize
131.97 KB
1.76 KB

All remaining test failures except for the 3 listed in #13+#14+#15 are like this:

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

Testing Drupal\Tests\aggregator\Functional\Update\AggregatorUpdateTest
F

Time: 36.9 seconds, Memory: 6.00MB

There was 1 failure:

1) Drupal\Tests\aggregator\Functional\Update\AggregatorUpdateTest::testSourceFeedRequired
No schema for rest.settings

/Users/wim.leers/Work/d8/core/tests/Drupal/Tests/SchemaCheckTestTrait.php:31
/Users/wim.leers/Work/d8/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:372
/Users/wim.leers/Work/d8/core/modules/aggregator/tests/src/Functional/Update/AggregatorUpdateTest.php:34

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

Process finished with exit code 1

Interestingly, this only happens for update path tests. My guess: because the default installation of D8 that update path tests run against does contain rest.settings configuration.

If I restore just

rest.settings:
  type: config_object

then the error changes to:

Schema key rest.settings:resources failed with: missing schema

Which points to rest_update_8201() + rest_post_update_create_rest_resource_config_entities(). Sure enough, when I add rest_update_8201(), tests pass again. (Or at lest AggregatorUpdateTest does.) I searched and found the minimal work-around necessary for now.

Wim Leers’s picture

Schema key rest.settings:bc_entity_resource_permissions failed with: missing schema

Looks like I literally have to restore the entire config schema.

Wim Leers’s picture

FileSize
3.33 KB
133.87 KB

This fixes the failures in EntitySerializationTest, Drupal\Tests\rest\Kernel\RequestHandlerTest and Drupal\Tests\rest\Functional\ResourceTest.

For the 3 remaining failures, see #13+#14+#15.

Current tally:

 69 files changed, 105 insertions(+), 2246 deletions(-)
Wim Leers’s picture

Title: [PP-2] Remove rest.module BC layers » [PP-1] Remove rest.module BC layers
Wim Leers’s picture

FileSize
7.74 KB
141.56 KB

The failing RestExportAuthTest was effectively an update path test, without it being named that way. Deleted it, and its test view.

71 files changed, 105 insertions(+), 2518 deletions(-)