Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
- General
All the@package
annotations are not something we do in Drupal core.- Annotation
-
The docblock doesn't follow the standard. For examples see \Drupal\ckeditor\Annotation\CKEditorPlugin + \Drupal\quickedit\Annotation\InPlaceEditor + \Drupal\rest\Annotation\RestResource.- The 'status' key. Ideally: remove (why does this exist?), otherwise: rename to 'status'.
- The 'data' key. This is a free-form storage. That's not what plugin definitions are designed to contain. So far, this only has 'prefix' and 'partialPath'. Why not make those explicit? What other use cases do you anticipate?
- The 'entityType' key. This limits JSON API to data stored in entities. Either that's intentional, and we can just remove JSON API plugins altogether (and generate routes directly etc), or we must remove this key.
- The 'bundle' key. Same.
- It seems additional plugin definition keys exist that are not listed: 'hasBundle', 'type', 'permission' and 'controller'.
- The "Bundle" JSON API resource
- Why not call it "entity bundle"? Just "bundle" makes little sense.
- The plugin deriver
-
The@see
inResourceDriver
is wrongResourceDriver
should be calledBundleDeriver
- The plugin interface
-
- It still has the default comment:
// Add get/set methods for your plugin type here.
. - Worse, the sole implementation of this interface has no methods:
class BundleJsonApiResource extends ContextAwarePluginBase implements JsonApiResourceInterface {}
… so if your plugins don't implement any methods, what purpose do they serve? This touches on the problems I pointed out in the "General" section about the 'entityType' and 'bundle' keys.
- It still has the default comment:
- The plugin namespace
- This is currently
jsonapi
— it probably should beJsonApiResource
. - The plugin manager
-
There's nothing to remark on
JsonApiResourceManager
… but
I first thought\Drupal\jsonapi\Configuration\ResourceManager(Interface)
was the plugin manager, but apparently it is not. It's something completely independent. I'm not sure the name of that class can be improved, but it's very similar, and hence confusing.
Parts that are struck through have already been addressed.
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff.txt | 4.1 KB | Wim Leers |
#30 | 2829398-30.patch | 139.08 KB | Wim Leers |
| |||
#27 | 2829398-27.patch | 135.66 KB | Wim Leers |
#21 | 2829398-21.patch | 135.92 KB | Wim Leers |
|
Comments
Comment #2
Wim LeersComment #3
Wim LeersComment #4
e0ipsoGreat insights!
This is probably a copy & paste leftover.
It is intentional. But we cannot remove plugins since resources can be toggled off.
This is used to detect the entity type of the current resource, without relying on a fixed URL structure.
My initial thought was to allow 3rd party modules to store unanticipated metadata about the resource.
Things like "I want to add a computed property to this resource, here is my callback". ATM this is just a placeholder.
Do you want me to get rid of it?
As I said we are allowing implementors to disable resources for certain entity types.
https://www.drupal.org/node/2781459#comment-11749367 contains an example on how to do so.
I'm good with that. I went for a shorter name for readability reasons, if you don't think it's a concern we can change it.
This should be added to the annotation.
Comment #5
Wim Leers\Drupal\jsonapi\Routing\Routes::routes()
and it's at\Drupal\jsonapi\Configuration\ResourceManager::all()
that you invoke the alter hook to allow specific entity JSON API resources to be turned off. None of that is in any way dependent on plugins.status
, for consistency with the rest of core.Comment #6
Wim LeersOh and apparently it's
\Drupal\jsonapi\Resource\EntityResource(Interface)
that contain the actual logic. If you want to keep plugins, I think it's this what should become the sole default plugin and the plugin interface. That could actually work. But again, I'd defer that to the future, when a real need arises to have JSON API for content other than entities.Comment #7
Wim LeersThis is by far the most important architectural issue that needs to be addressed.
Comment #8
Wim LeersTo add to #6:
\Drupal\jsonapi\Resource\EntityResource(Interface)
is what contains the actual logic, but then there is logic in\Drupal\jsonapi\RequestHandler
to actually call the appropriate method on thatEntityResource
class.So, the separation/coupling between:
RequestHandler
, which callsEntityResource
class, which contains the actual logic ofBundleJsonApiResource
plugin, which contains zero logic, and for which per-entity-type-and-bundle derivatives are generated byBundleDeriver
class, for which routes are generated by\Drupal\jsonapi\Routing\Routes
class, which makes the two aforementioned classes pretty much pointless…Comment #9
Wim LeersTaking this on.
In the mean time, already did #2831127: \Drupal\jsonapi\Plugin\FileDownloadUrl is not a plugin, links to wrong core issue, because that'll get in the way while working on this.
Comment #10
Wim LeersThis is essentially the direction. Unfortunately, lots of services use the
jsonapi.resource.manager
service, so I've got lots more refactoring ahead of me to actually make this work.Comment #11
Wim LeersWhat makes all of this at least ten times harder/more work than I thought, is the fact that code just about everywhere in the JSON API module is using
\Drupal\jsonapi\Configuration\ResourceConfig
objects. From the normalizers, to the link manager, to the JSON API context. It all retrieves metadata fromResourceConfig
objects. It's actually arguably one of the most important objects in the JSON API module. You just would never know by looking at the name.Still looking into how to deal with this. But it seems pretty clear that I can't get rid of it all. I'm still certain it can be made a whole lot clearer/simpler though.
Comment #12
Wim LeersSo, rather than getting rid of the plugin type/manager/annotation and
Resource(Config|Manager)
andEntityResource
, I just fixed it:JsonApiResource
annotation toJsonApiResourceType
, because for every resource type in JSON API, you can do CRUD on individual entities, but you can also access the collection of all resources of that type, and look at its relationships.BundleJsonApiResource
toEntityJsonApiResourcetype
, and moved all logic inEntityResource
into it. NowBundleJsonApiResource
is no longer an empty shell.JsonApiResourceTypeManager
(a plugin manager) only look for plugins in the JSON API module, which effectively means contrib can't add more JSON API Resource Type plugins.This is succesfully generating derived plugins and routes. Actual responses do not yet work.
Comment #13
Wim LeersI forgot to stage the changes for the annotation.
Comment #14
Wim LeersAnd this then is fully operational. There's definitely ugliness, but that's just because all of the existing normalizers either call out to "the current context", or call out to the resource manager (which is supposed to be entity-agnostic but now is no longer), and so on.
I took two shortcuts here:
So I think only the first is a real shortcut. And even then,
\Drupal\aggregator\Entity\Item
is the only entity type in core that doesn't support UUIDs (and it has an issue to fix that), so I think this is fine for now. UUIDs only really simplifies things, and is The Right Thing To Do for JSON API anyway.This patch (unlike the on in #12/#13) does need a lot of clean-up still.
Comment #15
Wim LeersOh, WTF.
Apparently JSON API in HEAD:
/api/node
route that returns all nodes, regardless of bundle (my patch above adds this)view
), it still generates aview--view
type instead of justview
, and worse, it generates a/api/view/view
route, instead of just/api/view
IMHO those things do not make sense. It does make the code a bit easier. But they don't make sense.
This cleans up part of what #14 did. And it makes the functional test coverage be green again. Many other tests will fail though.
Comment #17
Wim LeersSkip all those failing tests. Failing tests fail because they rely on HEAD's
ResourceConfig
andResourceManager
. Hence many tests need significant refactoring.Now this is blocked on feedback from mateu. Hopefully he agrees with the high-level direction and can push it to the finish line.
What's still left as a TODO, but which can be done in a follow-up IMO is:
EntityJsonApiResourceType::(getJsonApiResourceTypeForEntity|getEntityTypeIdForJsonApiResourceType|getBundleForJsonApiResourceType)
$context['resource_config']
to no longer receive the entire plugin definition, but just a JSON API resource type (e.g.node--article
)$context['resource_config']
to something more sensibleComment #19
Wim LeersUgh, IDK why, but
Notice: Unexpected PHP error: Use of undefined constant individual - assumed 'individual'
is fixed by this change.Comment #20
Wim LeersComment #21
Wim LeersDone.
Comment #22
e0ipsoI'm starting a more in depth review of this. In general, there is nothing stated in this thread I feel strongly against.
I don't like structured arrays in a well-known form, I prefer typed objects for those situations. So I'm unsure about the change of
$context['resource_config']
into a scalar (that can rapidly escalate to an array).Comment #23
e0ipsoThis is my partial review up to the
RelationshipNormalizerInterface
. I have to step out, but I'll continue the review later.One thing that seems clear to me is that we want the ResourceConfig be an object with different properties instead of doing brittle string manipulations everywhere. What do you think?
Code review:
I am ok about this, as long as we don't need to explode
$type
into$entity_type_id
and$bundle_id
, anywhere in the code.We should type hint the interface for testing & polymorphic purposes.
Maybe change to something like:
+1
Do we want to uncomment this? It seems like a good idea to me.
+1
Maybe: Which JSON API resource enpoint …
I'm more and more excited to drop support for entity IDs.
:clap:
Let's inject this somehow.
Good catch.
This is what I wanted to avoid in my other comment about the annotation.
I'd rather have the normalized components to generate the
node--page
when needed.Comment #24
e0ipsoThis is the rest of the review.
Should we remove this as well?
I realize that you said:
The biggest problem of doing so is here, with relationships. We need to be able to know if an entity_reference is a relationship to another resource or a UUID string (when there is no resource to point to).
@TODOne already?
Have only:
[…] allow additional types beyond entity type bundles.
Does this mean that we're planning to get definitions with a plugin ID that is not really a plugin ID (ie: without the derivative prefix)?
That seems a bit shady. It also gets in the way if we ever allow 3rd party JSON API resources.
Can we have an ID and deriver class name that hints that resources are not at the Entity Type level, but at the Bundle level?
I realize that naming may have not been ideal before either, but let's take the chance to improve on that.
I'd use an emoji to suggest dependency injection with a syringe, but d.o chokes with a 500 with emojis. I suspect on the MySQL table schema…
In any case, we should inject the service.
Woah! I almost don't remember when I wrote that… I agree, it needs to go away.
Are we missing
JsonApiResourceTypeManager
in the docblock? Maybe it's just an unfortunate diff.This is used by the custom route parameter upcaster to detect wrong entities. On a route for an
article
getting apage
is not allowed and should trigger a meaningful error.Why are we losing this?
Comment #25
e0ipsoI like the direction this goes. The only two things that I'd like to change are:
This is impressive work what you did here with so little time!! /me takes his hat off and bows.
How do you want to keep moving this forward?
Comment #26
Wim LeersWe discussed this during the weekly JSON API meeting.
ResourceConfig
objects. They're objects holding information about entity type ID + bundle, and know how to generate the corresponding JSON API type. They also know how to do reverse lookups (type -> entity type ID + bundle), to remove the need forexplode('--', $type)
.Comment #27
Wim LeersContinuing my work on this. First, rebasing to apply cleanly to HEAD again.
Comment #29
Wim Leers#26.3 was already done btw: #2836165: New experimental module: JSON API.
Comment #30
Wim LeersI resolved one conflict incorrectly.
And #2833850: IDs not converted to UUIDs for POST/PATCH to relationship endpoint introduced a bunch of code that required a fair amount of changes.
This should make the patch green again.
Comment #31
Wim LeersFirst: let's deal with "types" (e.g.
node--article
,user--user
, etc) versus entity type IDs + bundles. This is the most important piece of feedback probably, and the hardest to address.ResourceConfig
value object (better name TBD, per #26) MUST NOT be aware of entity types. Otherwise you still can't add new (non-entity) JSON API resource types.JsonApiResourceTypeManager::getDefinition('node--article')
. Besides making code elsewhere simpler, there's a more important reason: prevent the same JSON API resource type to be defined by multiple plugins! Because right now it's totally possible to haveentity:node--article
andfoobar:node--article
plugins. Which means we're not guaranteeing that one and only one plugin can define a particular JSON API resource type. This is hugely problematic.This is why I wrote this in the accompanying docblock:
Still trying to find a way to make this work. In the mean time, you can let me know whether you are convinced by these answers.
Comment #32
e0ipsoSlowly reviewing this. This caught my attention in the interdiff for #30.
It seems we are losing a test in here.
Comment #33
Wim Leers#32: you made the same remark at #24.11. That was explicitly testing whether a hook was able to set
enabled = FALSE
and whether that had the intended consequences. We agreed in #26.2 that that's no longer necessary/relevant.Comment #34
e0ipsoLOL. I repeat myself.
Sorry, I just need to get a good chunk of time to review this and help to move it forward.
Comment #35
e0ipsoI think this is the time to make an important decision. Let's lock the resources to bundles. One resource is tied one (and only one) entity type + bundle. Also, one entity type + bundle can only have one resource. Since there is no single ID that can help identify entity type + bundle, I suggest using
"$entity_type_id--$bundle"
to match thetype
of the resource. We can call this the resource ID from now on.Given a resource ID we should be able to build a plugin ID, maybe use the resource ID directly (?). A plugin for a resource should be aware of the entity type id and bundle id that the resource is for. This will effectively merge the responsibilities of ResourceConfig into the resource itself and reduce / eliminate the usage of the ResourceManager (which should only remain for relationships & related stuff).
I believe that this proposal will address #31.1, #31.2, #31.5 and make the code simpler.
Comment #36
Wim LeersI was working on moving forward on what I described in #31. But #35 means we're permanently tying the architecture of the JSON API module (at least for now) to .
Which means that the entire problem of having the JSON API module deal in the concept of "resources" and "relationships" etc goes away, because "resources" become "entities" (of a specific entity type ID + bundles) and "relationships" become "entity references".
It also means we no longer have to refactor (now or later) close to 100% of the architecture/internals of the JSON API module, because concepts like "entity collection" for example really should have been "resource collection", which just might happen to contain entities, "field resolver" should have been "attribute resolver" which just might happen to contain fields, and so on.
But, probably the strongest argument of all to design JSON API to only ever support entities for the foreseeable future: relationships & collections. It would be difficult to support collection queries that involve relationships to non-entity data.
Therefore, I support this.
Even more so because the JSON API module has no PHP APIs, it only offers #2835604: BigPipe provides functionality, not an API: mark all classes & interfaces @internal. This deliberate design choice, combined with a complete lack of configuration (also just like the BigPipe) module would make the JSON API module perfectly able to iterate on in the future: its internal architecture can be changed completely, as long as it provides the same exact HTTP API: the same URLs, the same supported requests, the same responses.
: a RESTful API. Which means we're effectively free to change the implementation at any time, theoretically even when it becomes stable. This is also the direction that the BigPipe module chose:So, I'm +1.
This does mean that we're throwing away pretty much everything in all the patches here. But that's not a bad thing! This would've been a momentously difficult patch to land anyway. We tried a different direction. We learned.
The good thing is we have agreement on many fronts:
ResourceConfig
value objects makes conceptual sense, but there are issues in the implementation as well as in the namingSo, moving forward on that common ground now, in separate child issues. That will make progress much easier.
To get started, here's a first child issue, with patch: #2838630: Remove the concept of "enabledness" from the JSON API module, rely on the Entity Access API instead. And in the process of working on that one, I already discovered another big bug that we definitely need to fix: #2838646: Relationships violate Entity Access API.
Comment #37
Wim LeersAdded #2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi' to remove the
prefix
configuration.Comment #38
Wim LeersAnd now finished up #2831134: Remove configurable 'id_field': always use UUID. Together with #2838580, this will mean we'll have zero configuration left :)
Comment #39
Wim LeersYay, #2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi' and #2831134: Remove configurable 'id_field': always use UUID have already landed! This means we have zero configuration left, and
\Drupal\jsonapi\Configuration\ResourceConfig
became slightly simpler.Next up is #2838630: Remove the concept of "enabledness" from the JSON API module, rely on the Entity Access API instead, which will make
ResourceConfig
again simpler.Comment #40
Wim Leers#2831134: Remove configurable 'id_field': always use UUID landed.
And #2838630: Remove the concept of "enabledness" from the JSON API module, rely on the Entity Access API instead also landed just now! We're making good progress here.
I think next up is for me to work on simplification/unification of the plugin stuff and the
ResourceConfig
stuff. I'll be on vacation next week, so expect more patches first week of January.Comment #41
Wim LeersIn order to clean up
ResourceConfig
, we must first ensure that it's the single source of truth, rather than the truth being spread overResourceConfig
and a plugin. So, let's refactor the plugin away.But in order to refactor the plugin away, we kind of need
ResourceConfig
objects to actually be value objects first, that'd make updatingRoutesTest
a lot easier.So, let's start with making
ResourceConfig
actual value objects: let them be immutable, and let them not have any services injected:Once those two land, we can do #2841056: Remove use of plugins.
Comment #42
Wim LeersThe first two have landed. Yay!
#2841056: Remove use of plugins is currently awaiting review + commit.
Next step: #2841287: Remove ResourceManager::hasBundle(), ResourceManager::getEntityTypeManager(), ResourceConfig::getPath(), and rename ResourceConfig::getBundleId().
Comment #43
Wim LeersAnd another next step: #2841296: Rename ResourceConfig & ResourceManager and remove their interfaces. This one is still being developed.
So the queue is now:
Comment #44
Wim Leers#2841056 is in. #2841287 is waiting to be committed. #2841296 is blocked on that, and is also ready.
Comment #45
Wim LeersComment #46
Wim LeersThe next and final (!) step is to address
EntityResource
+RequestHandler
(see #8) and the associated routes.Created #2841591: Remove EntityResource, move its logic into RequestHandler, clean up routes for that, which is blocked on #2841296: Rename ResourceConfig & ResourceManager and remove their interfaces.
Comment #47
Wim Leers#2841056: Remove use of plugins and #2841287: Remove ResourceManager::hasBundle(), ResourceManager::getEntityTypeManager(), ResourceConfig::getPath(), and rename ResourceConfig::getBundleId() are in!
Now we're down to the last two patches:
Comment #48
Wim LeersAfter those two patches are in, we can do #2842110: Meet Drupal coding standards.
Comment #49
e0ipsoEverything was marked as fixed. Fixing this plan as well.
Comment #50
Wim LeersIndeed! So glad to have this closed :)