Make ResourceTypeRepository aware of the path prefix

Hm, I don't like this one (yet). There's something cool about letting a resource type repository define an entire API surface, but it makes me worry about people (mis)using this internal "feature".

I am also concerned. If a resource type can provide a path prefix, that implies that five different resource types can have five different path prefixes. I doubt that that is what we want?

If you agree with that, then the only possible conclusion is that this is the wrong place for this abstraction to live.

Perhaps you're wondering where I then do think that this abstraction should live? I think making this a container parameter would be one viable approach: there can then only ever be one value, and it can be injected into any service that needs it. The jsonapi_extras module could then override it.

Also: #2878463: [PP-1] Define a JSON API link relation for entities is related.

We were incorrectly hard coding the resource URL in the Entity to JSON API service. This successfully abstracts that. On top of that this allows a massive simplification of JSON API Extras routing.

Yes, it's clearly an improvement in both cases :). I agree that it should be set differently than the status quo. I think my remaining concern is merely that I'm not yet convinced the repository is the best choice among the various alternatives.

I think it's semantically correct that the repository has an access route defined to it (the entry point).

This is the only piece of the change where I think I need more explanation/convincing/understanding.

+++ b/src/Routing/Routes.php
@@ -80,7 +80,8 @@ class Routes implements ContainerInjectionInterface {
+    $path_prefix = $this->resourceTypeRepository->getPathPrefix();

@Wim Leers, I think you're still partly suffering from the same initial misunderstanding I had. Note that it's the repository that defines the prefix, not each individual resource type.

That's slightly less concerning I think. I'm interested in the container idea, but I would need to know more about how that works to have a solid opinion.

I think you're still partly suffering from the same initial misunderstanding I had.

I don't think so. See https://cgit.drupalcode.org/jsonapi/commit/?id=4b5b103 — which was done in #2949635: Make the resource type aware of the resource path. (I don't know why it wasn't done as part of this issue, since it's semantically closely related.)

D'oh, I should learn to read: https://cgit.drupalcode.org/jsonapi/commit/?id=4b5b103 added ResourceType::getPath(), which excludes the path prefix. This is extremely confusing though, because usually "path" in the context of the web means "the path component of a URI", but here it means "the _last_part_ of the path component of a URI". (Which is related to the concern you raised at #2949635-7: Make the resource type aware of the resource path.)

I think we can make this all quite a bit clearer. Proposal in patch.

Or, if you prefer, \Drupal\jsonapi\ResourceType\ResourceTypeRepositoryInterface::getPrefixPathSegment() and \Drupal\jsonapi\ResourceType\ResourceType::getPathSegments(). That would match the terminology in https://tools.ietf.org/html/rfc3986#section-3.3.

Fix for #20.

+++ b/src/ResourceType/ResourceType.php
@@ -244,13 +244,15 @@ class ResourceType {
-    return sprintf('%s/%s', $this->getEntityTypeId(), $this->getBundle());
+    return sprintf('/%s/%s', $this->getEntityTypeId(), $this->getBundle());

+++ b/src/ResourceType/ResourceTypeRepository.php
@@ -77,7 +77,7 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
-    return 'jsonapi';
+    return '/jsonapi';

These changes address my only nit.

I'm assuming that because @Wim Leers added these patches, he is no longer in favor doing something with the container. Therefore, RTBC!

The container thing could still happen in the future, if we deem that better. This will do for now.

Thanks for #25, that helps, and makes a ton of sense! 👍

I think ResourceType::getPath() plus ResourceTypeRepository::getBasePath() would then probably be the best method name.

Implemented #26 based on #25. That also means fewer changes :)

LGTM.

I'd love to commit this, but I know this will impact jsonapi_extras. So I want to coordinate with @e0ipso to commit + release this simultaneously with jsonapi_extras.

This is now blocking #2971745: Don't hardcode `/jsonapi/` in FormatSetter, read JSON API base path from container parameter instead, which fixes buggy behavior for sites using jsonapi_extras.

I'd be fine doing this only in 2.x — I think that would help? :)

Are these changes worth all the hassle in your opinion?

Also, yes, I think this is worth the hassle. Especially if this goes into core.

Currently a string is returned, not a path prefix. And "path prefix" is really "base path" everywhere else in the web world (and in Drupal).

Furthermore, your commit here back in March actually breaks jsonapi_extras in situations where it chooses to override this base path/path prefix! That's why #2971745: Don't hardcode `/jsonapi/` in FormatSetter, read JSON API base path from container parameter instead exists. And it's blocked on this issue.

Finally: I wish you'd waited for feedback rather than just committed this back in March. 😞 Both Gabe and I responded quickly after the fact, but you let our responses linger :( If you'd have responded faster, or asked for feedback, we could've prevented the need for new releases of all those projects. I understand you just wanted to get things done, but if you want contributors to contribute, it's better to build consensus before committing. Let's do better from now on? :)

I don't really see much value in method naming or if a slash lives in a method or its caller.

I wouldn't care about this either if we weren't trying to get this into Drupal core.

I want to get things done right there and then because it's my only window of contribution.

I understand.

It's hard to reconcile those two positions, but hopefully we'll manage.

We always do! It's okay if things occasionally take longer.

I'll also push this commit to 2.x and then work on the blockers!

@e0ipso I'd love to get your thoughts on #2971745-14: Don't hardcode `/jsonapi/` in FormatSetter, read JSON API base path from container parameter instead.