Skip to content
This repository was archived by the owner on Sep 24, 2018. It is now read-only.

Conversation

@danielbachhuber
Copy link
Member

Same idea as #421, but different — this is a controller, not a model.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member Author

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?

@joehoyle
Copy link
Member

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 WP_JSON_Posts for example. You can extend that class and override methods where it makes sense and tie your class into a registered endpoint (or perhaps it's done through the parent class construct to be registered automatically). What is this offering over WP_JSON_Posts and associated posts, if we just want a "base" for all those classes, why not just add that in and abstract out some methods rather than writing a new here.

@danielbachhuber
Copy link
Member Author

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.

Yep. The remaining pieces to support are:

  • Checking context permissions
  • Registering relationships with other Resource types

What is this offering over WP_JSON_Posts and associated posts, if we just want a "base" for all those classes

Clean slate for discussion so we're talking conceptually and not fighting legacy ideas.

why not just add that in and abstract out some methods rather than writing a new here.

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 Resource and an informational endpoint.

I'm interested in @rmccue's opinion particularly too, because he didn't like the previous Resource implementation (which had some differences).

@rachelbaker rachelbaker added this to the 2.0 milestone Oct 29, 2014
Copy link
Member Author

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?

Copy link
Member Author

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

@danielbachhuber
Copy link
Member Author

💣

danielbachhuber added a commit that referenced this pull request Oct 29, 2014
Mock code to represent new Resources
@danielbachhuber danielbachhuber merged commit 01cbf4c into develop Oct 29, 2014
@danielbachhuber danielbachhuber deleted the mock-resource-class branch October 29, 2014 22:02
@danielbachhuber
Copy link
Member Author

Ooops, I meant 🚢

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants