store acceptable request handler in metadata #32
Conversation
|
Hey, @autarch what do you think, you use this module much more then I do and it has been a long time since I thought about this stuff. |
|
The basic idea seems right. The naming and implementation seem a bit odd. I'll make some comments on the patch page. |
| ($request->header('Content-Type') || 'application/octet-stream'), | ||
| $resource->content_types_accepted | ||
| ); | ||
| $metadata->{'acceptable_request_handler'} = $acceptable; |
There was a problem hiding this comment.
I think you want to store the return val of pair_value($acceptable), not the pair, which is what you're doing now.
There was a problem hiding this comment.
I think you want to store the return val of pair_value($acceptable), not the pair, which is what you're doing now.
Right. Changed the patch to reflect this:
|
Also, if the idea is to make this callable in your own code, we need to start treating _get_acceptable_content_type_handler as a method, not a sub. |
| ); | ||
| my $metadata = _metadata($request); | ||
| my $acceptable = $metadata->{'acceptable_request_handler'}; | ||
| if ( not defined $acceptable ) { |
There was a problem hiding this comment.
Use ! instead of not for consistency. Also, if you're going to store the sub (as I suggested) you can just check if (!$acceptable) since a subref is always defined (unless someone does something utterly insane with blessed subrefs and overloading, but that's their problem, not ours).
|
Oops - now I see the issue. On POST, _get_acceptable_content_type_handler is called only when post_is_create is true. So @autarch is right: I would need to call a get_acceptable_content_type_handler method in my process_post routine. |
|
The latest commit adds a get_acceptable_content_type_handler method to Web::Machine::Resource. The implementation is not pretty, but it does what I need and it doesn't break the test suite. |
|
As long as we are making a method to get the handler specified in content_types_accepted, we should also make one for content_types_provided. In my application I want to include an entity in every response, i.e. including responses with 4xx/5xx statuses, responses to POST when post_is_create is false, reponses to DELETE requests, etc. In other words, when Web::Machine does not call the handler for me, I need to call it myself. So in addition to a get_acceptable_content_type_handler method, which should extract the right handler from the array of hashref pairs returned by content_types_accepted, I also need a similar method that would extract the right handler from the array returned by content_types_provided. |
|
|
||
| sub get_acceptable_accept_handler { | ||
| my $self = shift; | ||
| Web::Machine::FSM::States::_get_acceptable_content_type_handler( $self, $self->request ); |
There was a problem hiding this comment.
This seems to be identical to get_acceptable_content_type_handle
|
@autarch Thanks for the review. I will go over this again and re-push. |
Hi guys, further to https://rt.cpan.org/Public/Bug/Display.html?id=101203 here is a patch that stores the acceptable request handler in the metadata.
While this does reduce the number of calls to 'content_types_accepted', that is not the justification. I believe this change is needed because Web::Machine does not call the request handler when 'post_is_create' is false. Instead, it calls 'process_post' which I have to implement myself. There I would like to call the acceptable request handler (which has been known since B5), except I have no way of obtaining it. Hence this patch.