Skip to content

[WIP] #15502 Make template shortcuts be usable without Templating component #15620

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

Closed
wants to merge 2 commits into from

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Aug 26, 2015

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? ?
Fixed tickets #15502
License MIT
Doc PR -

@dosten
Copy link
Contributor

dosten commented Aug 26, 2015

You should add this change to the CHANGELOG.md

@fabpot
Copy link
Member

fabpot commented Aug 26, 2015

👍

@Tobion
Copy link
Contributor

Tobion commented Aug 26, 2015

👍 but some tiny tests to ensure these methods keep being called would be good.

@Koc
Copy link
Contributor Author

Koc commented Aug 26, 2015

@Tobion can you give advice, maybe some piece of code how to do this? Bootstrap kernel without templating - where should I do it?

@Tobion
Copy link
Contributor

Tobion commented Aug 26, 2015

I don't mean functional tests but unit tests. We already have unit tests for this class. Just mock the container and ensure the methods are called.

$twig = $this->container->get('twig');

$callback = function () use ($twig, $view, $parameters) {
$twig->loadTemplate($view)->display($parameters);
Copy link
Member

Choose a reason for hiding this comment

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

use $twig->display() here rather than the internal implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

loadTemplate() is public, so it's not internal

Copy link
Member

Choose a reason for hiding this comment

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

But what's the point of using the more verbose version?

Copy link
Contributor

Choose a reason for hiding this comment

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

one less method call ;)
doesn't really matter to me. I'm fine with both

@dosten
Copy link
Contributor

dosten commented Aug 27, 2015

And if the twig service isn't available? IMO the code should be something like this:

if ($this->container->has('templating')) {
    // use the templating component
} else if ($this->container->has('twig')) {
    // use twig
} else {
    throw new \LogicException('You can not use the XXX method if the Templating Component or the Twig Bundle are not available.');
}

@fabpot
Copy link
Member

fabpot commented Sep 14, 2015

Thank you @Koc.

@fabpot fabpot closed this in 46eaafc Sep 14, 2015
@fabpot
Copy link
Member

fabpot commented Sep 14, 2015

I've reverted the tests

@Koc Koc deleted the twig-shortcuts branch October 2, 2015 15:29
@fabpot fabpot mentioned this pull request Nov 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants