Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Adding parameters to templates before render-time #143

Merged
merged 15 commits into from
Oct 10, 2015

Conversation

kynx
Copy link
Contributor

@kynx kynx commented Sep 29, 2015

My first run at #142. Only tried it with Plates so far. There's probably a better way to do this with ZendView, but I'm a little hazy on that. Let me know if I'm on the right track and I'll do the tests and docs.

* @param array|object $params
* @param string|null $name
*/
public function addParameters($params, $name = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method also needs to be added to the interface; otherwise, an implementation could omit it, leaving developers depending on concretions instead of abstractions.

@weierophinney
Copy link
Member

Some general notes:

  • Terminology needs to be consistent. You have $templateParams, addParameters(), and mergeParams() — three different ways ways of referencing related things. $name is unclear as an argument; are you indicating a parameter name, or a template name? Additionally, these are default parameters to use, not the only parameters. As such, I'd go with:
    • private $defaultParams = []
    • public function setDefaultParams(array $params, $templateName = null) (more on this method later)
    • private function mergeParams(array $params, $templateName = null)
  • I'd shy away from public methods that work with a collection, such as addParameters(). In my experience, these methods cause a fundamental confusion: does it merge or overwrite an existing set of parameters? One way of solving this is to do as we did in PSR-7 with attributes. We omitted withAttributes(), and instead provided only withAttribute() and withoutAttribute(); the first overwrites or creates the single name header, while the second removes it. I suggest doing the same here:
    • public function addDefaultParam($templateName, $name, $value)
    • public function removeDefaultParam($templateName, $name)
  • Related to the previous point, I'd require the template name, and provide a constant in the template interface for specifying "all templates"; users who want to specify a global default template parameter would use something like $renderer->addDefaultParam($renderer::ALL_TEMPLATES, 'url', $url).

Finally, you'll need to rebase, as #139 renamed TemplateInterface to TemplateRendererInterface, and appended the name Renderer to all impementations.

With regards to the ZF2 implementation, I'd handle it in createModel(), merging the default parameters with those passed in.

@weierophinney weierophinney added this to the 0.4.0 milestone Oct 9, 2015
@weierophinney weierophinney self-assigned this Oct 9, 2015
@kynx
Copy link
Contributor Author

kynx commented Oct 10, 2015

Thanks for the feedback! I'll get on with that then.

@weierophinney
Copy link
Member

@kynx I've been working on the suggestions this morning as well; I can do a PR against your branch shortly, if you'd like.

kynx added 7 commits October 10, 2015 15:18
…ive into template-add-parameter

* 'template-add-parameter' of github.com:kynx/zend-expressive:
  Added Plates tests
  CS fix
  Fix for adding shared params to Plates. Default for template name now null, not empty string.
  Fix for adding shared params to Plates. Default for template name now null, not empty string.
  Fixed failing tests
  TemplateInterface::addParameters() method for building up parameters before rendering
@kynx
Copy link
Contributor Author

kynx commented Oct 10, 2015

@weierophinney Please do! Looks like I messed up the rebase tho :(

weierophinney added a commit to weierophinney/zend-expressive that referenced this pull request Oct 10, 2015
Adding parameters to templates before render-time
weierophinney added a commit to weierophinney/zend-expressive that referenced this pull request Oct 10, 2015
- Adds the method `addDefaultParam($templateName, $key, $value)` to
  `TemplateRendererInterface`; define a TEMPLATE_ALL constant.
  Replaces `addParameters()`.
- Renames `AddParametersTrait` to `DefaultParamsTrait`
  - Renames `$templateParams` to `$defaultParams`
  - Renames `addParameters()` to `addDefaultParam()`
- Updates PlatesRenderer to the interface changes.
- Modifies Twig and ZendView renderers to use the new trait.
- Wrote tests for the behavior for each renderer.
weierophinney added a commit to weierophinney/zend-expressive that referenced this pull request Oct 10, 2015
- Adds the method `addDefaultParam($templateName, $key, $value)` to
  `TemplateRendererInterface`; define a TEMPLATE_ALL constant.
  Replaces `addParameters()`.
- Renames `AddParametersTrait` to `DefaultParamsTrait`
  - Renames `$templateParams` to `$defaultParams`
  - Renames `addParameters()` to `addDefaultParam()`
- Updates PlatesRenderer to the interface changes.
- Modifies Twig and ZendView renderers to use the new trait.
- Wrote tests for the behavior for each renderer.
weierophinney added a commit to weierophinney/zend-expressive that referenced this pull request Oct 10, 2015
@weierophinney
Copy link
Member

@kynx — I've created #151 so I could run Travis-CI checks; at this point, those are all passing. I've also added documentation of the new interface method.

We have two options at this point:

Let me know!

@weierophinney weierophinney merged commit 7cdab23 into zendframework:develop Oct 10, 2015
weierophinney added a commit that referenced this pull request Oct 10, 2015
@weierophinney
Copy link
Member

@kynx I've gone ahead and merged both branches at this time; thanks for the new feature!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants