TemplateFactory: add support for custom Template implementation #159

mrtnzlml · 2016-09-25T10:11:31Z

With this change it's possible to simply use custom Template implementation. Before this commit Template object initialization was hard-coded into TemplateFactory (related issue: #141). Usage example (config.neon):

latte:
    templateClass: App\CustomTemplateImplementation

Custom template implementation MUST implement Nette\Application\UI\ITemplate interface.

feature
issues - TemplateFactory - variable template class (RFC) #141
documentation - probably not needed because this part of framework is not documented (internals)
BC break - no

This is more request for comments than real PR. Thank you.

TomasVotruba · 2016-09-25T17:45:59Z

src/Bridges/ApplicationLatte/TemplateFactory.php

 		$this->user = $user;
 		$this->cacheStorage = $cacheStorage;
+
+		\Nette\Utils\Validators::assert($templateClassName, 'string');


This is not needed, as it is not convention in Nette.
Also PHP 7 is comming soon with type checks.

It's not convention in Nette doesn't mean I cannot do that, does it? And it's not true that it's not needed (before PHP 7). Moreover PHP 7 is not used in Nette yet.

Don't add it here please. Code would be inconsistent with other strings (scalar) types in methods.

Rather check if this class implements ITemplate interface. Because it would fail later and now it silently pass in here creating invalid object.

I have to do both checks because without checking string it's possible to call new on integer for example. From my point of view validation is needed. Can you help me how to prevent calling $template = new $this->templateClassName($latte); (line 57) with wrong $this->templateClassName type?

I see. If so, add them both here. It doesn't make sense splitting them.
Standalone method would be best, e.g. validateTemplateClassName().

Ok, I'll improve it in next iteration. I am used to doing validations in constructor in my projects...

Yes. This should be in constructor.

Something like this:

public function __construct($className = Tempalte::class) { $this->validateTemplateClassName($className); $this->className = $className; } private function validateTemplateClassName($className) { // is string // implements ITemplate // throw exception otherwise }

So move this runtime check https://github.com/nette/application/pull/159/files#diff-0bc2c99180eba473648d41663a94e15cR58 there

~~I don't see benefit here. It's only because of calling line Nette\Utils\Validators::assert($templateClassName, 'string'); here. I don't like it... :)~~

~~Waiting for more opinions...~~

(outdated comment)

We missunderstood a bit. I've updated the code to explain better. Is it more clear now?

Yeah, it cannot be done like that. It's because $template is created during createTemplate call (everytime you call it) and therefore you cannot test its instance in constructor. You don't have ITemplate in constructor. What implements ITemplate? The answer is: created $template.

TomasVotruba · 2016-09-25T17:47:20Z

src/Bridges/ApplicationLatte/TemplateFactory.php

 	{
 		$latte = $this->latteFactory->create();
-		$template = new Template($latte);
+		$template = new $this->templateClassName($latte);


This looks ok. Just wondering about use case, when custom class has different arguments.

It should be in another PR if needed I think. It's not possible to add another arguments with current implementation as well.

Ok, let's skip it now then.

Btw, do you need $latte as argument in your implementation?

Not necessarily because I am extending Nette\Bridges\ApplicationLatte\Template and then calling $this->getLatte(). So the answer is no but original implementation needs it.

TomasVotruba · 2016-09-25T17:47:39Z

tests/Bridges.Latte/TemplateFactory.customTemplate.phpt

+Assert::same([], $template->flashes);
+Assert::same('ok', $template->render());
+$template->setFile('bla');
+Assert::same('alb', $template->render());


Awesome test, thank you.

mrtnzlml · 2016-09-25T18:05:43Z

tests/Bridges.Latte/TemplateFactory.customTemplate.phpt

+{
+	private $file = 'ko';
+
+	public function render() {


TODO: fix coding standards

JanTvrdik · 2016-09-25T21:47:43Z

Setter (as suggested in #141) is IMHO better (template class name has a character of configuration not of dependency)

mrtnzlml · 2016-09-25T21:59:27Z

@JanTvrdik I love this idea (despite the naming - just idea) even more. But I agree with you.

JanTvrdik · 2016-09-26T06:52:11Z

You mean the „TemplateClassFactory“ which would create just instance of Template without configuring it any further? That seems like a terrible idea to me.

Good solutions are either setter or splitting the instance creation and configuration to multiple methods to allow overriding part of the logic.

TomasVotruba · 2016-09-26T06:58:22Z

I agree with @JanTvrdik in setter as configuration.

If you want to change class name, you might want to do it multiple times. With constructor dependency you would have to create new instance of TemplateFactorry.

mrtnzlml · 2016-09-26T07:26:54Z

~~TODO for me: Change implementation back to the setter one. Make it configurable using DI extension if possible (because original usage is not nice).~~ (done)

mrtnzlml · 2016-09-26T13:30:22Z

Ok guys, what about this implementation?

mrtnzlml · 2016-09-26T13:31:46Z

src/Bridges/ApplicationDI/LatteExtension.php

 	public $defaults = [
 		'xhtml' => FALSE,
 		'macros' => [],
+		'templateClass' => \Nette\Bridges\ApplicationLatte\Template::class


Is this coupling between bridges fine?

JanTvrdik · 2016-09-26T13:35:46Z

src/Bridges/ApplicationLatte/TemplateFactory.php

 		$latte = $this->latteFactory->create();
-		$template = new Template($latte);
+		$template = new $this->templateClass($latte);
+		if (!$template instanceof UI\ITemplate) {


This check should be moved to setTemplateClass (see is_a with third argument set to TRUE)

You will also probably need to call class_exists on the $templateClass to load the class to memory

Thanks - I didn't know the third argument of is_a.

JanTvrdik · 2016-09-26T13:35:49Z

src/Bridges/ApplicationLatte/TemplateFactory.php

 		return $template;
 	}

+	public function changeTemplateClass($templateClass)


should be named setTemplateClass (due to consistency)

JanTvrdik · 2016-09-26T13:35:51Z

src/Bridges/ApplicationLatte/TemplateFactory.php


+	public function changeTemplateClass($templateClass)
+	{
+		\Nette\Utils\Validators::assert($templateClass, 'string');


should be removed (due to consistency)

How would you prevent passing something different than string? I understand your note about consistency, but it's wrong I think. Or should I remove usage of Validators and use custom check?

How would you prevent passing something different than string?

Here is the secret – you don't prevent passing something different because this is PHP and because it would kill code readability and performance. Pretty much every Nette method without typehint can be broken by passing the wrong type.

JanTvrdik · 2016-09-26T13:39:26Z

src/Bridges/ApplicationDI/LatteExtension.php

 	public $defaults = [
 		'xhtml' => FALSE,
 		'macros' => [],
+		'templateClass' => \Nette\Bridges\ApplicationLatte\Template::class


should be simplified to Nette\Bridges\ApplicationLatte\Template::class

the usecase se imho too narrow to add option to the extension so I would remove this entirely

if added, the default value should be NULL and the setter call should only be emitted if the value is not NULL

mrtnzlml · 2016-09-26T14:15:16Z

Updated - ~~I have no idea why is old conversation still here (what a mess in this discussion)~~... :-/
What changed:

it uses setter implementation
it's still configurable via Latte DI extension
it's still used validation in setTemplateClass because I strongly believe that we should check if passed class name is string (and not created instance for example)

Thanks for help.

JanTvrdik · 2016-09-26T14:26:33Z

src/Bridges/ApplicationLatte/TemplateFactory.php

+	{
+		\Nette\Utils\Validators::assert($templateClass, 'string');
+		if (!is_a($templateClass, UI\ITemplate::class, TRUE)) {
+			throw new \Nette\Utils\AssertionException('Template should be instance of ' . UI\ITemplate::class . ", $templateClass given.");


should be Nette\InvalidArgumentException, imho

you need to call !class_exist($templateClass) || before !is_a because is_a does not trigger autoloading

JanTvrdik · 2016-09-26T14:26:47Z

src/Bridges/ApplicationDI/LatteExtension.php

+		$templateFactory = $builder->addDefinition($this->prefix('templateFactory'))
 			->setClass(Nette\Application\UI\ITemplateFactory::class)
 			->setFactory(Nette\Bridges\ApplicationLatte\TemplateFactory::class);
+		if($config['templateClass'] !== NULL) {


missing space after if

mrtnzlml · 2016-09-26T14:44:49Z

Updated according to comments.

JanTvrdik · 2016-09-26T15:09:46Z

src/Bridges/ApplicationLatte/TemplateFactory.php

+	public function setTemplateClass($templateClass)
+	{
+		if (!class_exists($templateClass) || !is_a($templateClass, UI\ITemplate::class, TRUE)) {
+			throw new \Nette\InvalidArgumentException('Template should be instance of ' . UI\ITemplate::class . ", $templateClass given.");


Nette\InvalidArgumentException

"Class '$templateClass' does not implement ... ."

dg · 2016-09-26T18:24:09Z

src/Bridges/ApplicationLatte/TemplateFactory.php


+	public function setTemplateClass($templateClass)
+	{
+		if (!class_exists($templateClass) || !is_a($templateClass, UI\ITemplate::class, TRUE)) {


It should be instance of Template::class

Why not ITemplate? Can you please elaborate it more @dg? I don't fully understand... :) Thank you.

My guess is that it's because TemplateFactory has @return Template annotation. However Nette itself does not seem to rely on the instance being Template.

It has @return Template, it assumes __construct(Latte\Engine) and __set for variables (or fields).

With this change it's possible to simply use custom Template implementation. Before this commit Template object initialization was hard-coded into TemplateFactory (related issue: #141). Usage example (config.neon): latte: templateClass: App\CustomTemplateImplementation Custom template implementation MUST implement Nette\Application\UI\ITemplate interface.

mrtnzlml · 2016-11-06T21:57:59Z

Rebased on top of master branch (because of failing tests).

mrtnzlml · 2016-12-20T10:42:48Z

Thank you @dg! :)

…oses #159][Closes #141] With this change it's possible to simply use custom Template implementation. Usage example in config.neon: latte: templateClass: App\CustomTemplateImplementation Custom template MUST extend Nette\Bridges\ApplicationLatte\Template.

dg · 2016-11-07T00:48:58Z

src/Bridges/ApplicationLatte/TemplateFactory.php


+	public function setTemplateClass($templateClass)
+	{
+		if (!class_exists($templateClass) || !is_a($templateClass, Template::class, TRUE)) {


IMHO is_a(..., ..., TRUE) triggers autoloader. Otherwise it is PHP bug.

dg · 2016-11-07T00:50:53Z

src/Bridges/ApplicationLatte/TemplateFactory.php

+	public function setTemplateClass($templateClass)
+	{
+		if (!class_exists($templateClass) || !is_a($templateClass, Template::class, TRUE)) {
+			throw new Nette\InvalidArgumentException("Class $templateClass does not extend " . Template::class . ' or it does not exist.');


without it

dg · 2016-11-07T00:52:46Z

src/Bridges/ApplicationLatte/TemplateFactory.php


 		return $template;
 	}



missing empty line

dg · 2016-12-19T11:23:14Z

src/Bridges/ApplicationDI/LatteExtension.php

 			->setClass(Nette\Application\UI\ITemplateFactory::class)
 			->setFactory(Nette\Bridges\ApplicationLatte\TemplateFactory::class);
+		if ($config['templateClass'] !== NULL) {
+			$templateFactory->addSetup('?->setTemplateClass(?)', ['@self', $config['templateClass']]);


Why not simply $templateFactory->addSetup('setTemplateClass', [$config['templateClass']]);?

TomasVotruba reviewed Sep 25, 2016

View reviewed changes

TomasVotruba mentioned this pull request Sep 25, 2016

TemplateFactory - variable template class (RFC) #141

Closed

mrtnzlml commented Sep 25, 2016

View reviewed changes

mrtnzlml changed the title ~~TemplateFactory: add support for custom Template implementation~~ WIP: TemplateFactory: add support for custom Template implementation Sep 26, 2016

mrtnzlml commented Sep 26, 2016

View reviewed changes

JanTvrdik suggested changes Sep 26, 2016

View reviewed changes

mrtnzlml changed the title ~~WIP: TemplateFactory: add support for custom Template implementation~~ TemplateFactory: add support for custom Template implementation Sep 26, 2016

dg reviewed Sep 26, 2016

View reviewed changes

dg closed this in 2d565db Dec 20, 2016

mrtnzlml deleted the customTemplate2 branch December 20, 2016 10:42

dg reviewed May 2, 2017

View reviewed changes

Uh oh!

TemplateFactory: add support for custom Template implementation #159

TemplateFactory: add support for custom Template implementation #159

Uh oh!

Conversation

mrtnzlml commented Sep 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomasVotruba Sep 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomasVotruba Sep 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrtnzlml Sep 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomasVotruba Sep 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanTvrdik commented Sep 25, 2016

Uh oh!

mrtnzlml commented Sep 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JanTvrdik commented Sep 26, 2016

Uh oh!

TomasVotruba commented Sep 26, 2016

Uh oh!

mrtnzlml commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrtnzlml commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanTvrdik Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanTvrdik Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mrtnzlml commented Sep 25, 2016 •

edited

Loading

TomasVotruba Sep 25, 2016 •

edited

Loading

TomasVotruba Sep 25, 2016 •

edited

Loading

mrtnzlml Sep 25, 2016 •

edited

Loading

TomasVotruba Sep 25, 2016 •

edited

Loading

mrtnzlml commented Sep 25, 2016 •

edited

Loading

mrtnzlml commented Sep 26, 2016 •

edited

Loading

JanTvrdik Sep 26, 2016 •

edited

Loading

JanTvrdik Sep 26, 2016 •

edited

Loading

mrtnzlml commented Sep 26, 2016 •

edited

Loading