Add an inline_layout stash keyword, based on the mojo.js inlineLayout one. #1887
Conversation
mkende
force-pushed
the
mojo_js_inline_layout
branch
from
2c65608
to
2d74508
Compare
|
This shoud be ready for another round of review at your convenience (I’m not sure what notification is sent by Github). |
mkende
force-pushed
the
mojo_js_inline_layout
branch
from
f387dc2
to
77c1cab
Compare
|
Thanks, please take another look. (I’m not sure why the CI is failing, the messages seem unrelated to the change as they are about some docker initialisation error) |
|
Any chance to get a new review? I believe that I have addressed all your comment and that this might be ready to be merged. |
lib/Mojolicious/Renderer.pm
Outdated
| @@ -108,6 +108,10 @@ sub render { | |||
| # Inheritance | |||
| my $content = $stash->{'mojo.content'} //= {}; | |||
| local $content->{content} = $output =~ /\S/ ? $output : undef if $stash->{extends} || $stash->{layout}; | |||
| if ($stash->{inline_layout}) { | |||
| @$options{inline} = delete $stash->{inline_layout}; | |||
There was a problem hiding this comment.
@$options doesn't make sense here.
There was a problem hiding this comment.
I can't quite say what yet, but something about the logic seems wrong too. 🤔
There was a problem hiding this comment.
Maybe that inline value needs to be local-ised to avoid side effects?
There was a problem hiding this comment.
You’re right that it should be possible to execute another layout defined in the inline layout (if we started from a non-inline template).
I localized the inline value and fixed another bug (a layout and inline_layout defined initially could have both been executed), and I observed that it prevented entering the second loop. However, I was not able to get it to work in the correct case (template => inline_layout => other layout defined in the inline_layout), so I was not able either to write a good test case for the fixed part.
lib/Mojolicious/Renderer.pm
Outdated
| @@ -108,6 +108,10 @@ sub render { | |||
| # Inheritance | |||
| my $content = $stash->{'mojo.content'} //= {}; | |||
| local $content->{content} = $output =~ /\S/ ? $output : undef if $stash->{extends} || $stash->{layout}; | |||
| if ($stash->{inline_layout}) { | |||
mkende
force-pushed
the
mojo_js_inline_layout
branch
from
77c1cab
to
9578a17
Compare
mkende
force-pushed
the
mojo_js_inline_layout
branch
from
9578a17
to
d7eb0a7
Compare
|
Let me know if there are still issues to address here. |
|
If there are no more comments on this PR, could it be merged? |
| if $stash->{extends} || $stash->{layout} || $stash->{inline_layout}; | ||
| if (defined $stash->{inline_layout}) { | ||
| local $options->{inline} = delete $stash->{inline_layout}; | ||
| delete $stash->{layout}; |
There was a problem hiding this comment.
Do the tests still pass if this line is removed?
There was a problem hiding this comment.
They do. I tried to write something to test this line but did not succeed (setting layout and inline_layout together in a call to render results in an error 500 but I’m not sure why). I still have the impression that this line should stay.
Do you have an idea of how to test it? Or do you think that it should be removed?
There was a problem hiding this comment.
Code that does nothing should not be present.
|
Too bad this PR is not ready, i would have liked to ship it in the next release. |
Summary
This adds a way to pass an inline layout in calls to render() using a new
inline_layoutkeyword.Motivation
Using the mojo.js semantics to allow inline layout seems to be the safest way.
References
See the discussion in #1780.