Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TemplateFactory: add support for custom Template implementation #159

Closed
wants to merge 1 commit into from

Conversation

@mrtnzlml
Copy link
Contributor

mrtnzlml commented Sep 25, 2016

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 - #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.

{
$this->latteFactory = $latteFactory;
$this->httpRequest = $httpRequest;
$this->user = $user;
$this->cacheStorage = $cacheStorage;

\Nette\Utils\Validators::assert($templateClassName, 'string');

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba Sep 25, 2016

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

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 25, 2016

Author Contributor

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.

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba Sep 25, 2016

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.

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 25, 2016

Author Contributor

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?

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba Sep 25, 2016

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

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 25, 2016

Author Contributor

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

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba Sep 25, 2016

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

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 25, 2016

Author Contributor

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)

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba Sep 25, 2016

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

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 25, 2016

Author Contributor

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.

@@ -48,7 +54,11 @@ public function __construct(ILatteFactory $latteFactory, Nette\Http\IRequest $ht
public function createTemplate(UI\Control $control = NULL)
{
$latte = $this->latteFactory->create();
$template = new Template($latte);
$template = new $this->templateClassName($latte);

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba Sep 25, 2016

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

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 25, 2016

Author Contributor

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

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba Sep 25, 2016

Ok, let's skip it now then.

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

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 25, 2016

Author Contributor

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.

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

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba Sep 25, 2016

Awesome test, thank you.

{
private $file = 'ko';

public function render() {

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 25, 2016

Author Contributor

TODO: fix coding standards

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Sep 25, 2016

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

@mrtnzlml

This comment has been minimized.

Copy link
Contributor Author

mrtnzlml commented Sep 25, 2016

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

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Sep 26, 2016

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

This comment has been minimized.

Copy link

TomasVotruba commented Sep 26, 2016

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 mrtnzlml changed the title TemplateFactory: add support for custom Template implementation WIP: TemplateFactory: add support for custom Template implementation Sep 26, 2016
@mrtnzlml

This comment has been minimized.

Copy link
Contributor Author

mrtnzlml commented Sep 26, 2016

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

This comment has been minimized.

Copy link
Contributor Author

mrtnzlml commented Sep 26, 2016

Ok guys, what about this implementation?

@@ -19,6 +19,7 @@ class LatteExtension extends Nette\DI\CompilerExtension
public $defaults = [
'xhtml' => FALSE,
'macros' => [],
'templateClass' => \Nette\Bridges\ApplicationLatte\Template::class

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 26, 2016

Author Contributor

Is this coupling between bridges fine?

@@ -48,7 +51,11 @@ public function __construct(ILatteFactory $latteFactory, Nette\Http\IRequest $ht
public function createTemplate(UI\Control $control = NULL)
{
$latte = $this->latteFactory->create();
$template = new Template($latte);
$template = new $this->templateClass($latte);
if (!$template instanceof UI\ITemplate) {

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Sep 26, 2016

Contributor

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

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Sep 26, 2016

Contributor

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

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 26, 2016

Author Contributor

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

@@ -112,4 +119,10 @@ public function createTemplate(UI\Control $control = NULL)
return $template;
}

public function changeTemplateClass($templateClass)

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Sep 26, 2016

Contributor

should be named setTemplateClass (due to consistency)

@@ -112,4 +119,10 @@ public function createTemplate(UI\Control $control = NULL)
return $template;
}

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

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Sep 26, 2016

Contributor

should be removed (due to consistency)

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 26, 2016

Author Contributor

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?

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Sep 26, 2016

Contributor

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.

@@ -19,6 +19,7 @@ class LatteExtension extends Nette\DI\CompilerExtension
public $defaults = [
'xhtml' => FALSE,
'macros' => [],
'templateClass' => \Nette\Bridges\ApplicationLatte\Template::class

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Sep 26, 2016

Contributor
  • 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

This comment has been minimized.

Copy link
Contributor Author

mrtnzlml commented Sep 26, 2016

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.

{
\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.");

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Sep 26, 2016

Contributor
  • should be Nette\InvalidArgumentException, imho
  • you need to call !class_exist($templateClass) || before !is_a because is_a does not trigger autoloading
->setClass(Nette\Application\UI\ITemplateFactory::class)
->setFactory(Nette\Bridges\ApplicationLatte\TemplateFactory::class);
if($config['templateClass'] !== NULL) {

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Sep 26, 2016

Contributor

missing space after if

@mrtnzlml

This comment has been minimized.

Copy link
Contributor Author

mrtnzlml commented Sep 26, 2016

Updated according to comments.

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.");

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Sep 26, 2016

Contributor
  • Nette\InvalidArgumentException
  • "Class '$templateClass' does not implement ... ."
@mrtnzlml mrtnzlml changed the title WIP: TemplateFactory: add support for custom Template implementation TemplateFactory: add support for custom Template implementation Sep 26, 2016
@@ -112,4 +115,12 @@ public function createTemplate(UI\Control $control = NULL)
return $template;
}

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

This comment has been minimized.

Copy link
@dg

dg Sep 26, 2016

Member

It should be instance of Template::class

This comment has been minimized.

Copy link
@mrtnzlml

mrtnzlml Sep 26, 2016

Author Contributor

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

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Sep 26, 2016

Contributor

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.

This comment has been minimized.

Copy link
@dg

dg Sep 26, 2016

Member

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

This comment has been minimized.

Copy link
Contributor Author

mrtnzlml commented Nov 6, 2016

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

@dg dg closed this in 2d565db Dec 20, 2016
@mrtnzlml

This comment has been minimized.

Copy link
Contributor Author

mrtnzlml commented Dec 20, 2016

Thank you @dg! :)

@mrtnzlml mrtnzlml deleted the mrtnzlml:customTemplate2 branch Dec 20, 2016
dg added a commit that referenced this pull request Dec 20, 2016
…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 added a commit that referenced this pull request Dec 20, 2016
…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.
@@ -112,4 +115,12 @@ public function createTemplate(UI\Control $control = NULL)
return $template;
}

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

This comment has been minimized.

Copy link
@dg

dg May 2, 2017

Member

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

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.');

This comment has been minimized.

Copy link
@dg

dg May 2, 2017

Member

without it

@@ -112,4 +115,12 @@ public function createTemplate(UI\Control $control = NULL)
return $template;
}

This comment has been minimized.

Copy link
@dg

dg May 2, 2017

Member

missing empty line

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

This comment has been minimized.

Copy link
@dg

dg May 2, 2017

Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.