-
Notifications
You must be signed in to change notification settings - Fork 18
Provide stateless template variable aggregation #70
Provide stateless template variable aggregation #70
Conversation
Sure ;) I'll test that after eating and give you my feedback ;) You're work is much appreciated. |
@@ -456,6 +463,138 @@ $app->get('/download/tarball', [ | |||
], 'download-tar'); | |||
``` | |||
|
|||
### Template Variable Container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should not be in the README file and must be moved to the documentation. The informations in the README and the documentation are already different for this topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@froschdesign I'll be doing a separate PR to the zend-expressive repo's documentation once this is merged and released. Unfortunately, we have the docs for this package in that repo, so we need to have something in the README here as well.
I'd like to eventually move some of the repo-specific docs out of the zend-expressive docs, but I'm not 100% sure how we can do that while keeping the information findable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we have the docs for this package in that repo, so we need to have something in the README here as well.
Keep it simple: Add a link to the documentation to the README file - as everywhere. In this particular case, I would also add a short note that the informations can be found in the zend-expressive docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weierophinney
Please compare these two middleware and you will see the problem that I mean:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@froschdesign Noted.
I'll keep it as-is for now, and then, after the release, submit a PR to the zend-expressive repo with documentation. After that is merged, I'll send another PR here to remove the verbiage and link to the official docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be a merge recursive?
*/ | ||
public function mergeForTemplate(array $values) : array | ||
{ | ||
return array_merge($this->variables, $values); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be a recursive merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if there was a good use case for that. Generally speaking, you assign discrete values to templates: scalars, possibly objects, occasionally arrays. I'm not sure it would make sense to do a recursive merge for arrays, as you would generally be replacing the value at the top-level assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weierophinney You're right ;)
BTW: I would want require your branch through composer with commit ref but it seem I cannot access it. I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because the patch is submitted via my fork, and not as a branch on this repo (I submit PRs the same as anybody else does! 😄). As such, you will need to add a repositories
section to your composer.json
:
"repositories": [
{"type": "vcs", "url": "https://github.com/weierophinney/zend-expressive-helpers.git"}
]
Then, where you require
zend-expressive-helpers in your composer.json
, update it as follows:
"zendframework/zend-expressive-helpers": "dev-feature/template-variable-container as 5.2.0",
Then run composer update
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the repository thing, I was aware of ;) The problem: I was not able to find your fork through your github profile ;) I must be blind...
However, I was not aware that you can alias a fork to a specific released version for dependencies purpose. That really great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, you have a typo I think in your comment ;)
"zendframework/zend-expressive-json": "dev-feature/template-variable-container as 5.2.0",
JSON ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have a typo I think in your comment
fixed.
@weierophinney I'm not sure but I think that the current implementation is close to template. Let's talk about a request handler: $content = $this->renderer->render(
'some::template',
$container->mergeForTemplate([
'local' => 'value',
])
); So basically put here, we are aggregating variables to a page template. Ok, but what if we want access those variables in the layout? The above variables will be close to the page template, not accessible from the layout. This was not the purpose of this feature after all? Or should we pre-render the layout inside the handler too? That sound a bit strange to me. |
Ok... the feature (the variable container) will be close to the page template. Why? Layout is automatically rendered when the page template is rendered and in the Plates template, there is no check to see if the layout has been already rendered. However, you can, from the page template, pass variables to the layout... OUCH.... For layout variables which are common to all pages, that seem a bit tedious but... |
|
||
class RouteTemplateVariableMiddlewareTest extends TestCase | ||
{ | ||
public function setUp() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protected
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be either protected
or public
. We're not terribly consistent in the framework in terms of using one or the other. I typically define this and tearDown()
as public.
@nuxwin —
For variables common to all pages, you would use a delegator factory on the The Yes, it does mean that you will need to pass parameters for the layout from your template. You can simplify this in a number of ways, depending on the template engine you use:
My point is that there are a number of ways to make this work without requiring a ton of effort. The primary advantage is that it makes the system stateless, more explicit, and thus more predictable, all of which are necessary for async scenarios. Is it as easy as using |
Isn't the solution to make
|
Yes, but not in that way! If you return a new instance, you would then need to pass that instance via a request attribute to the middleware or handler that finally renders a template... which gets complicated quickly! The approach we would need to take is two-fold:
The other thing to remember: using |
@weierophinney Well, there are imho two contexts for injecting default params. Container (factory, delegator) and middleware/handler. My quick idea (which is bc break) - "split"
This two should be used by template implementations and thus container context. Than both approaches could be solved. Just my two cents :-) |
Right - which is exactly the use case for the feature in this patch. What's interesting about the approach is that it can be used for more than just templates; it could be used to aggregate request-specific values for any context. (Which makes me think that perhaps it should potentially get a different name, to allow for re-use between contexts.) That said, there's still a reason for |
Sure, I had the same idea as soon as I wrote my last post. But I think this new variable container should be stateless.
I agree. I hope Thumbs up. |
Um, why? The use case it is specifically trying to solve is how to aggregate variables representing request state for use with rendering a template when in an async environment where aggregating state in a service has side-effects. (I'm wondering if we're approaching separate problems?) |
It's my personal feeling - I do not like objects which could be changed somewhere under my hands. Second point of view - proposed variable container is just smart |
`TemplateRendererInterface::addDefaultParam()` has side-effects, and should not be used in async environments unless setting template variables that will be used on **every** request. As such, this patch introduces an alternative workflow for aggregating pipeline-specific template variables. It provides two classes: - `Zend\Expressive\Helper\Template\TemplateVariableContainer`, which provides a way to aggregate template variables, as well as merge local variables for the purpose of producing a set to pass to the template renderer. - `Zend\Expressive\Helper\Template\TemplateVariableContainerMiddleware`, which provides a way to register the container as a request attribute within your pipeline. Middleware can then set template parameters using either the container's `set()` or `merge()` methods: ```php $request->getAttribute(TemplateVariableContainer::class) ->set('user', $user); $request->getAttribute(TemplateVariableContainer::class) ->merge([ 'user' => $user, 'route' => $route, ); ``` Handlers can then use the `mergeForTemplate()` method to prepare variables to pass to the renderer: ```php $content = $this->renderer->render( 'some::template', $container->mergeForTemplate([ 'handler-specific-variable' => $value, ]) ); ```
This patch builds on the previous patch, which introduced the TemplateVariableContainer and associated middleware. This middleware can replace the `UrlHelperMiddleware`, and be used to set the discovered route from the `RouteResult` as the template variable `route`.
Per a suggestion from @vaclavvanik, I have refactored the TemplateVariableContainer to be immutable. `set()` was renamed to `with()`, and `unset()` to `without()`. Each of these methods, as well as `merge()`, now return a new instance cloned from the previous, with the changes requested. This also meant that the RouteTemplateVariableMiddleware needed updates in order to reset the `TemplateVariableContainer` request attribute.
…emplateVariableMiddleware This patch modifies the behavior of RouteTemplateVariableMiddleware in the following ways: - It now *always* populates a `TemplateVariableContainer` instance with a `route` parameter. This means if none was in the request previously, it will create one (via the default argument to `getAttribute()`). - It populates the value with the `RouteResult`, which allow access to not just the matched route, but also any matched parameters. It also means users can test to see if a match actually occured. These changes were made in light of zendframework/zend-expressive-platesrenderer#36, which adds a `route()` template method, which will return the `RouteResult` (not the route, nor the route name; the route result).
43505de
to
0981643
Compare
@vaclavvanik I've made Considering this is a standard package for Expressive applications, this is a reasonable tradeoff. |
This patch updates the README and CHANGELOG to reflect current state of the new classes.
TemplateRendererInterface::addDefaultParam()
has side-effects, and should not be used in async environments unless setting template variables that will be used on every request.As such, this patch introduces an alternative workflow for aggregating pipeline-specific template variables. It provides two classes:
Zend\Expressive\Helper\Template\TemplateVariableContainer
, which provides a way to aggregate template variables, as well as merge local variables for the purpose of producing a set to pass to the template renderer.Zend\Expressive\Helper\Template\TemplateVariableContainerMiddleware
, which provides a way to register the container as a request attribute within your pipeline.Middleware can then set template parameters using either the container's
set()
ormerge()
methods:Handlers can then use the
mergeForTemplate()
method to prepare variables to pass to the renderer:Additionally, this patch provides
Zend\Expressive\Helper\Template\RouteTemplateVariableMiddleware
. This middleware works in conjunction with theTemplateVariableContainerMiddleware
, and inspects the request for aZend\Expressive\Router\RouteResult
instance. If found, it injects the value returned bygetMatchedRoute()
under the nameroute
in theTemplateVariableContainer
, allowing templates access to it via the variable$route
. This value will either be empty, or aZend\Expressive\Router\Route
instance.The
RouteTemplateVariableMiddleware
can be used in place of theUrlHelperMiddleware
, particularly if you do not use theUrlHelper
without a route name.