👍

Chance for test? :)

Will try for the first change, at least. Still searching for the current file to add the test in.

This is within an around modifier for a Moo attribute which should allow $response->content to get the current content (if ever needed) but the current logic doesn't handle that well. I'd prefer an approach like the following, but this fails a couple of tests (which I'm chasing).

around content => sub {
    my ( $orig, $self ) = ( shift, shift );

    # called as getter
    @_ or return $self->$orig;

    # no serializer defined; encode content and set attribute
    $self->serializer
        or return $self->$orig( $self->encode_content(@_) );

    # serialize content and set attribute
    my $serialized = $self->serialize(@_);
    $self->is_encoded(1); # All serializers return byte strings
    return $self->$orig( defined $serialized ? $serialized : '' );
};

If you think it can take some time, please merge my PR first for a release, and then fix it correctly later. This is making some plugins to fail (like Dancer2::Plugin::Database).

Merry Christmas (after two days, but better later then never)

And merry Christmas to you @ambs 😄

Git blame indicates that it is the work on allowing before serializer hooks ( #1053 ) that changed the behaviour here (see commit 5697359). My suggestion above almost undoes the changes in the around modifier in that commit, and the test failures I'm seeing were changes as a result of that commit. Looks like I need to write some more test cases first... (ping @mickeyn for thoughts)

Yeah, my commit didn't take the "0" case into account - so it's good to check the length (we can also check the content-length header to make sure we don't read content unless it's positive.

BTW, I have a local commit waiting to become PR that removes the 'around content' and makes it a nice 'ro' attribute with a writer (cause f*#!@ 'around's).

$response->content gets called as a getter when the psgi response is being constructed. It doesn't make sense to be calling serializer again. Either the around modifier needs to check its being called as a getter, or replaced (best idea!).

@mickeyn did you have any tests for what the changes from #1053 allow ?

👍 everyone. :)

The hooks tests were updated to comply with the change (which basically covers the 'always called' behavior).

Do you think we're missing coverage there?

@mickeyn yes: tests for an app with a before_serializer hook sets the content when nothing was returned from a route, but leaves the content to be serialized alone otherwise.

The changes to the around content modifier in commit 5697359 "always serialize", including if called as a getter. When the PSGI response arrayref is constructed within Dancer2::Core::Respose->to_psgi, it calls $self->content which triggers serialization again (actually twice, as there are two calls.). But when called as a getter there is no content so the hook in the suggested test case would replace whatever content was in the response (failing the test).

See https://github.com/PerlDancer/Dancer2/blob/master/lib/Dancer2/Core/Response.pm#L183
I suspect what's needed is the default empty content added there should be serialized if a serializer is defined, plus the around modifier changes to be reverted. But its late and I'm going to sleep on it..

Closing this now that #1082 has been merged.