1. ResourceManager::hasBundle() has zero uses left after #2841056: Remove use of plugins
  2. ResourceManager::getEntityTypeManager has some uses, but … it doesn't make sense that we're using the manager of resources to get a certain service. For that, we have dependency injection. The classes that need this service, should just have this service injected!
  3. ResourceConfig::getPath() is used only in \Drupal\jsonapi\Routing\Routes. Which strongly suggests this is a value that does NOT belong in the ResourceConfig value object.
  4. ResourceConfig::getBundleId() should be called ResourceConfig::bundle() — all of Drupal core uses the term bundle, not bundle ID — JSON API should be consistent with that.

The end result is that:

  1. ResourceManager(Interface) is much more focused: it has only a all() method (to get all resources) and a get() method (to get a specific resource)
  2. ResourceConfig(Interface) is more focused: one less method, and one method that's now consistent with the rest of core

In other words: pure clean-up & simplification.

CommentFileSizeAuthor
#17 interdiff--13-16.txt688 bytese0ipso
#13 jsonapi_simplify_resourcemanager-2841287-13.patch36.12 KBwim leers
#13 interdiff.txt12.71 KBwim leers
#6 interdiff.txt12.34 KBwim leers
#6 jsonapi_simplify_resourcemanager-2841287-6.patch28.31 KBwim leers
#4 jsonapi_simplify_resourcemanager-2841287-4.patch17.75 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Remove ResourceManager::hasBundle() and ResourceManager::getEntityTypeManager() » [PP-1] Remove ResourceManager::hasBundle() and ResourceManager::getEntityTypeManager()
Status: Active » Postponed

This is blocked on #2841056: Remove use of plugins.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2841056: Remove use of plugins
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new jsonapi_simplify_resourcemanager-2841287-4.patch17.75 KB

Slight simplification: 11 files changed, 59 insertions, 82 deletions

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Issue tags: +blocker

This is blocking #2841296: Rename ResourceConfig & ResourceManager and remove their interfaces.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Remove ResourceManager::hasBundle() and ResourceManager::getEntityTypeManager() » [PP-1] Remove ResourceManager::hasBundle(), ResourceManager::getEntityTypeManager(), ResourceConfig::getPath(), and rename ResourceConfig::getBundleId()
Issue summary: View changes
StatusFileSize
new jsonapi_simplify_resourcemanager-2841287-6.patch28.31 KB
new interdiff.txt12.34 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Now quite a bit of simplification: 15 files changed, 91 insertions, 140 deletions

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Title: [PP-1] Remove ResourceManager::hasBundle(), ResourceManager::getEntityTypeManager(), ResourceConfig::getPath(), and rename ResourceConfig::getBundleId() » Remove ResourceManager::hasBundle(), ResourceManager::getEntityTypeManager(), ResourceConfig::getPath(), and rename ResourceConfig::getBundleId()
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Postponed » Needs review
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Needs work 
  1. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
@@ -118,10 +130,7 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
-      $entities = $this->resourceManager->getEntityTypeManager()
-        ->getStorage($entity_type_id)
-        ->loadByProperties(['uuid' => $value['id']]);
-      $entity = reset($entities);
+      $entity = $this->entityRepository->loadEntityByUuid($entity_type_id, $value['id']);

    Is it really worth it to add a whole new dependency to load an entity by UUID? Given that you can do it with the entity type manager and you need it a anyways… 

  2. +++ b/src/Configuration/ResourceConfig.php
@@ -97,11 +83,10 @@ class ResourceConfig implements ResourceConfigInterface {
+    $this->bundle = $bundle_id;

    Should we also rename all occurrences of $bundle_id to $bundle?

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

I'm unsure about:

it doesn't make sense that we're using the manager of resources to get a certain service. For that, we have dependency injection. The classes that need this service, should just have this service injected!

Dependency injection is a glorified bag of global variables. Grabbing a copy of it multiple times or once makes little difference at a technical level. For me these changes introduce a bit of cruft (which is against the spirit of simplification). Additionally, we agree that a JSON-API resource is intimately coupled to an entity type and a bundle. It makes sense to me that a ResourceManager can provide the EntityTypeManager.

Thoughts?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#10.1: but this class does NOT use the entity type manager! It uses only the entity repository. Because all it needs, is to load entities. That's what the entity repository is for.

#10.2: Yes, we should. Good catch! I indeed forgot to update the local variables. Will reroll in the morning.

#11: The point of a container is that there is a single source of truth. Every time you're getting access to a service via something else, you risk being out of sync with the container (in case a service is overridden) — which is why code stopped doing that because it introduced subtle bugs. It also makes unit testing more difficult. It also makes it more difficult to see what the *actual* dependencies of a service are. And finally: this is simply a hard requirement in core! I challenge you to find a single service in core that has a public "getSomeOtherService" method!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt12.71 KB
new jsonapi_simplify_resourcemanager-2841287-13.patch36.12 KB

Done.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

but this class does NOT use the entity type manager!

Correct! Sorry, I didn't read it right.

in case a service is overridden [after the resource manager grabs it]

This got me convinced. Let's do this!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yay :)

  • e0ipso committed ef83d38 on 8.x-1.x authored by Wim Leers 
    Remove ResourceManager::hasBundle(), ResourceManager::...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Fixed
StatusFileSize
new interdiff--13-16.txt688 bytes

I think the interdiff in #13 is reversed, anyways the code looks good.

This was committed with a minor change to the patch in #13. See interdiff.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Oops, and thanks! :)

Status: Fixed » Closed (fixed)

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