-
Notifications
You must be signed in to change notification settings - Fork 651
Mock code to represent new Resources #549
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$args['id'] is a special variable defined by the route, so we can expect it always exists to get this far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As is $args['taxonomy']. For many of the existing term functions, we can depend on core internals throwing an error if we've provided invalid data.
lib/class-wp-json-resource.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WP_HTTP_Request is a new object that would have the full request arguments, access to the server as needed, headers, etc. etc. It's patterned after other libraries (jQuery, Symphony) which provide a "simplified" array of request arguments to start, but still give you access to the full request data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the Dependency Injection that Nacin pushed us to already remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, can you clarify further, or link me to the conversation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I disagree with this being dependency injection; it's really just a bag of params and other request-related information to read, rather than something that does useful work (typically what DI is used for).
Let's split this conversation to #552.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should all Resources be required to accept it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, would you expect that you're always inspecting context from the WP_HTTP_Request object, and that context should no longer be passed?
|
Hey @danielbachhuber, This looks pretty clear and defined I'd say, and if I understand correctly is just a "normal" class abstraction to build a concept of "resources" (something you can GET, UPDATE, and DELETE) in a standardised manor. I'm sold on the need for that, and also now this doesn't tie us into anything, you would presumably just register the endpoint in some manor like #551 so there is no "magic" there. However, I'm not sure how this differs really to what already exists with |
Yep. The remaining pieces to support are:
Clean slate for discussion so we're talking conceptually and not fighting legacy ideas.
If/when this PR is merged, the existing classes can be renamed, methods tweaked slightly, and extend this new abstract class. Taxonomies needs to be retooled anyway because it needs to be broken into a I'm interested in @rmccue's opinion particularly too, because he didn't like the previous |
lib/class-wp-json-resource.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rachelbaker @rmccue @joehoyle how do you feel the fact that this is passed the $item and not the request args? should it also receive the request object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this to Controller
|
💣 |
Mock code to represent new Resources
|
Ooops, I meant 🚢 |
Same idea as #421, but different — this is a controller, not a model.