prooph / psr7-middleware

Consume prooph messages with a PSR7 middleware
http://getprooph.org/
BSD 3-Clause "New" or "Revised" License
14 stars 8 forks source link

add command, event, query and message middleware #1

Closed sandrokeil closed 8 years ago

sandrokeil commented 8 years ago

This PR is ready for a review now. :tada: /cc @prolic @codeliner

prolic commented 8 years ago

I had only a short review so far. It looks very simple and intuitive. Good work!

codeliner commented 8 years ago

review done. Looks really great. Let's clarify the question in my comment and then it is ready to merge :+1:

sandrokeil commented 8 years ago

@codeliner Yes, using parsed body for the QueryMiddleware is wrong, but possible. Merging of query params and request params is not a good choice, see this example result:

result = {array} [8]
 action = "Prooph\Psr7Middleware\QueryMiddleware"
 email = "test@domain.com"
 name = "prooph"
 originalUri = {Zend\Diactoros\Uri} [9]
 prooph_query_name = "Prooph\ProophessorDo\Model\User\Query\UserList"
 REQUEST_METHOD = "GET"
 user_id = "f1a9d1c9-30eb-4352-82d6-4a899409f741"
 Zend\Expressive\Router\RouteResult = {Zend\Expressive\Router\RouteResult} [5]

What do you think about the solution to add a prooph_attribute_list request parameter, cycle through this list and add the parameter from the request to the payload? To support @prolic request body, we could add a additional attribute prooph_use_request_body and merge the payload with the parsed request body data. The request body overwrites all previous payload data, if defined.

Here is some pseudo code

$attributeList = [];

foreach ($request->getAttribute(self::NAME_ATTRIBUTE_LIST) as $attribute)
{
    $attributeList[$attribute] = $request->getAttribute($attribute);
}

$query = $this->queryFactory->createMessageFromArray(
    $queryName,
    ['payload' => array_merge($request->getQueryParams(), $attributeList, $requestBody]
);

@prolic This middleware can not handle a query param sort and a body param sort, because the body param overwrites the query param or is this ok in your case?

prolic commented 8 years ago

No it's not ok, and when you use $_POST and $_GET instead of $_REQUEST no value gets overridden. You handle them separately and you can have separate values. The psr-7 request can also distinguish between get and post. Am 09.02.2016 09:51 schrieb "Sandro Keil" notifications@github.com:

@codeliner https://github.com/codeliner Yes, using parsed body for the QueryMiddleware is wrong, but possible. Merging of query params and request params is not a good choice, see this example result:

result = {array} [8] action = "Prooph\Psr7Middleware\QueryMiddleware" email = "test@domain.com" name = "prooph" originalUri = {Zend\Diactoros\Uri} [9] prooph_query_name = "Prooph\ProophessorDo\Model\User\Query\UserList" REQUEST_METHOD = "GET" user_id = "f1a9d1c9-30eb-4352-82d6-4a899409f741" Zend\Expressive\Router\RouteResult = {Zend\Expressive\Router\RouteResult} [5]

What do you think about the solution to add a prooph_attribute_list request parameter, cycle through this list and add the parameter from the request to the payload? To support @prolic https://github.com/prolic request body, we could add a additional attribute prooph_use_request_body and merge the payload with the parsed request body data. The request body overwrites all previous payload data, if defined.

Here is some pseudo code

$attributeList = [];foreach ($request->getAttribute(self::NAME_ATTRIBUTE_LIST) as $attribute){ $attributeList[$attribute] = $request->getAttribute($attribute);}$query = $this->queryFactory->createMessageFromArray( $queryName, ['payload' => array_merge($request->getQueryParams(), $attributeList, $requestBody]);

@prolic https://github.com/prolic This middleware can not handle a query param sort and a body param sort, because the body param overwrites the query param or is this ok in your case?

— Reply to this email directly or view it on GitHub https://github.com/prooph/psr7-middleware/pull/1#issuecomment-181787029.

sandrokeil commented 8 years ago

@prolic But how do you handle the values of $_POST and $_GET in the payload? I think you must use an own middleware for this behaviour and we only use the approach with query params and attribute list. For a REST API it would not work to use a POST to retrieve data. So we should use the HTTP specification behaviour (GET requests have no body).

prolic commented 8 years ago

Rest api with cqrs is weird anyway. As i said i use get for generic query info, like query name, sort, filter, pagination and post for something like user id param needed for the query. If you won't change it i will use my own implementation as it's more flexible. Am 09.02.2016 10:09 schrieb "Sandro Keil" notifications@github.com:

@prolic https://github.com/prolic But how do you handle the values of $_POST and $_GET in the payload? I think you must use an own middleware for this behaviour and we only use the approach with query params and attribute list. For a REST API it would not work to use a POST to retrieve data. So we should use the HTTP specification behaviour (GET requests have no body).

— Reply to this email directly or view it on GitHub https://github.com/prooph/psr7-middleware/pull/1#issuecomment-181795873.

sandrokeil commented 8 years ago

@prolic The default behaviour for the QueryMiddleware is to use query params now. However, I've added the support for POST HTTP requests and the parsed body data is stored under the key data.

Can we merge it now? /cc @codeliner