New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add request object #563

Merged
merged 12 commits into from Nov 2, 2014

Conversation

Projects
None yet
3 participants
@rmccue
Member

rmccue commented Nov 1, 2014

This is essentially a bag of everything relating to the request.

Fixes #552.

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Nov 1, 2014

Member

@WP-API/amigos #reviewmerge

Member

rmccue commented Nov 1, 2014

@WP-API/amigos #reviewmerge

@rmccue rmccue added this to the 2.0 milestone Nov 1, 2014

rmccue added some commits Nov 1, 2014

Include params parsed from the route
Also changes get_param to use POST (if relevant), then GET, then the URL
parameters. This mirrors the previous behaviour from v1.
protected $route;
/**
* Attributes (options) for the route that was matched

This comment has been minimized.

@danielbachhuber

danielbachhuber Nov 1, 2014

Member

What are some example options?

@danielbachhuber

danielbachhuber Nov 1, 2014

Member

What are some example options?

This comment has been minimized.

@rmccue

rmccue Nov 2, 2014

Member

Added documentation; this is the array used in the registration.

@rmccue

rmccue Nov 2, 2014

Member

Added documentation; this is the array used in the registration.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Nov 1, 2014

Member

Generally, I like this idea a lot. The conservative side of me wonders if it's too magical for core.

@nacin @markjaquith opinions? To summarize, the default argument to our new WP_JSON_Controller class is object implementing ArrayAccess so it can also be used as an array.

Member

danielbachhuber commented Nov 1, 2014

Generally, I like this idea a lot. The conservative side of me wonders if it's too magical for core.

@nacin @markjaquith opinions? To summarize, the default argument to our new WP_JSON_Controller class is object implementing ArrayAccess so it can also be used as an array.

*
* @var string
*/
protected $method = '';

This comment has been minimized.

@danielbachhuber

danielbachhuber Nov 1, 2014

Member

Should this default to GET?

@danielbachhuber

danielbachhuber Nov 1, 2014

Member

Should this default to GET?

This comment has been minimized.

@rmccue

rmccue Nov 2, 2014

Member

I think it's better to not default here so that it's evident if it hasn't been set. I'd even be happy with defaulting to null.

@rmccue

rmccue Nov 2, 2014

Member

I think it's better to not default here so that it's evident if it hasn't been set. I'd even be happy with defaulting to null.

// Compatibility for clients that can't use PUT/PATCH/DELETE
if ( isset( $_GET['_method'] ) ) {
$this->method = strtoupper( $_GET['_method'] );
$request->set_method( strtoupper( $_GET['_method'] ) );

This comment has been minimized.

@danielbachhuber

danielbachhuber Nov 1, 2014

Member

Should the strtoupper() happen within set_method() so we can be sure it always conforms? I see you're already using it twice here.

@danielbachhuber

danielbachhuber Nov 1, 2014

Member

Should the strtoupper() happen within set_method() so we can be sure it always conforms? I see you're already using it twice here.

This comment has been minimized.

@TimothyBJacobs

TimothyBJacobs Nov 1, 2014

+1 to sanitizing inside the set method.

@TimothyBJacobs

TimothyBJacobs Nov 1, 2014

+1 to sanitizing inside the set method.

This comment has been minimized.

@rmccue

rmccue Nov 1, 2014

Member

I agree, I kept it this way to match PSR-7 though. I've no strong argument either way, but it's nice being compatible-ish with PSR-7.

@rmccue

rmccue Nov 1, 2014

Member

I agree, I kept it this way to match PSR-7 though. I've no strong argument either way, but it's nice being compatible-ish with PSR-7.

This comment has been minimized.

@TimothyBJacobs

TimothyBJacobs Nov 2, 2014

Does PSR-7 mean that two strings with the same value, but different case are different methods?

@TimothyBJacobs

TimothyBJacobs Nov 2, 2014

Does PSR-7 mean that two strings with the same value, but different case are different methods?

Show outdated Hide outdated plugin.php
*
* This ensures that the request is consistent.
*
* @param array|WP_JSON_Request $reques Request to check.

This comment has been minimized.

@danielbachhuber

danielbachhuber Nov 1, 2014

Member

Typo: $reques

@danielbachhuber

danielbachhuber Nov 1, 2014

Member

Typo: $reques

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Nov 2, 2014

Member

Updated as per review.

Generally, I like this idea a lot. The conservative side of me wonders if it's too magical for core.

The ArrayAccess is a bit magical, but I don't think it's any worse than things like WP_Post using the post fields or falling back to meta otherwise.

I'm going to go ahead and merge this, as I think we're OK with the idea as a whole, and we can PR any other changes we want here.

Member

rmccue commented Nov 2, 2014

Updated as per review.

Generally, I like this idea a lot. The conservative side of me wonders if it's too magical for core.

The ArrayAccess is a bit magical, but I don't think it's any worse than things like WP_Post using the post fields or falling back to meta otherwise.

I'm going to go ahead and merge this, as I think we're OK with the idea as a whole, and we can PR any other changes we want here.

rmccue added a commit that referenced this pull request Nov 2, 2014

@rmccue rmccue merged commit 5478d4f into joes-registration Nov 2, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@rmccue rmccue deleted the two-request-object branch Nov 2, 2014

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