Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Also, this is a net reduction: 8 files changed, 48 insertions, 310 deletions :)

Wim Leers’s picture

  1. +++ b/jsonapi.services.yml
    @@ -91,9 +91,6 @@ services:
    -  plugin.manager.resource.processor:
    -    class: Drupal\jsonapi\Plugin\JsonApiResourceManager
    -    parent: default_plugin_manager
    

    Remove the plugin manager.

  2. +++ /dev/null
    @@ -1,79 +0,0 @@
    -class ResourceDeriver extends DeriverBase implements ContainerDeriverInterface {
    

    Remove the sole plugin's deriver.

  3. +++ /dev/null
    index 560b703..0000000
    --- a/src/Plugin/JsonApiResourceInterface.php
    

    Remove the plugin interface.

  4. +++ b/src/Routing/Routes.php
    @@ -52,13 +52,13 @@ class Routes implements ContainerInjectionInterface {
    -    $this->resourcePluginManager = $resource_plugin_manager;
    ...
    +    $this->resourceManager = $resource_manager;
    

    Switch from the plugin manager to the resource manager.

  5. +++ b/src/Routing/Routes.php
    @@ -97,70 +94,62 @@ class Routes implements ContainerInjectionInterface {
    +        ->setRequirement('_bundle', $resource_config->getBundleId())
    ...
    -      if ($bundle) {
    -        $route_collection->setRequirement('_bundle', $bundle);
    

    Turns out this is a simplification we can make here.

  6. +++ b/src/Routing/Routes.php
    @@ -79,16 +79,13 @@ class Routes implements ContainerInjectionInterface {
    -    foreach ($this->resourcePluginManager->getDefinitions() as $plugin_id => $plugin_definition) {
    -      $entity_type = $plugin_definition['entityType'];
    -      // For the entity type resources the bundle is NULL.
    -      $bundle = $plugin_definition['bundle'];
    -      $partial_path = $plugin_definition['data']['partialPath'];
    -      $route_keys = explode(':', $plugin_id);
    -      $route_key = end($route_keys) . '.';
    -      // Add the collection route.
    +    foreach ($this->resourceManager->all() as $resource_config) {
    
    @@ -97,70 +94,62 @@ class Routes implements ContainerInjectionInterface {
    -        ->setRequirement('_entity_type', $entity_type)
    -        ->setRequirement('_permission', $plugin_definition['permission'])
    +        ->setRequirement('_entity_type', $resource_config->getEntityTypeId())
    ...
    +        ->setRequirement('_permission', 'access content')
    

    This converts from using the plugin manager to the resource manager. This is basically the key change.

    This is also basically the only thing we need to change when we drop the use of plugins.

    Which just goes to show how little we used plugins :)

Wim Leers’s picture

FileSize
1.1 KB
24.68 KB

Hah, apparently I forgot to remote the annotation.

The net reduction is now even better: 9 files changed, 48 insertions, 364 deletions :)

e0ipso’s picture

Title: [PP-2] Remove use of plugins » [PP-1] Remove use of plugins
Wim Leers’s picture

Title: [PP-1] Remove use of plugins » Remove use of plugins
Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: jsonapi_remove_plugin-2841056-6.patch, failed testing.

Wim Leers’s picture

Because #2841050: Make ResourceConfig an actual value object: immutable, with no services injected was modified since creating #6, the patch in #6 no longer applies. Here's a rebased patch.

Wim Leers’s picture

Status: Needs work » Needs review

Dammit, cross-posted.

Status: Needs review » Needs work

The last submitted patch, 10: jsonapi_remove_plugin-2841056-9.patch, failed testing.

Wim Leers’s picture

Ah, of course, I rebased the patch so it applies, but the changes in #2841050-15: Make ResourceConfig an actual value object: immutable, with no services injected require another change to happen here too. Patch coming…

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
26.07 KB
4.05 KB

And done.

Wim Leers’s picture

The rationale explanation in #5 still applies. #10 and #14 were only necessary to adjust for the modifications to the patch in #2841050: Make ResourceConfig an actual value object: immutable, with no services injected made after #2+#6 were created.

Wim Leers’s picture

e0ipso’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/src/Routing/Routes.php
@@ -97,70 +94,62 @@ class Routes implements ContainerInjectionInterface {
-      if ($bundle) {
-        $route_individual->setRequirement('_bundle', $bundle);
-      }

I checked, and this change does not conflict with the \Drupal\jsonapi\Routing\RouteEnhancer. So, :+1:

I'm merging this.

  • e0ipso committed b711d24 on 8.x-1.x authored by Wim Leers
    fix(Maintainability) Remove use of plugins (#2841056 by Wim Leers)
    
e0ipso’s picture

Status: Reviewed & tested by the community » Fixed
Wim Leers’s picture

Yay!

Status: Fixed » Closed (fixed)

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