Use namespaced Twig by nicolas-grekas · Pull Request #22962 · symfony/symfony

nicolas-grekas · 2017-05-30T13:33:46Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	needs twigphp/Twig#2490 and twigphp/Twig#2491
Fixed tickets	-
License	MIT
Doc PR	-

nicolas-grekas · 2017-05-30T13:39:05Z

src/Symfony/Bridge/Twig/DataCollector/TwigDataCollector.php

+use Twig\Markup;
+use Twig\Profiler\Dumper\HtmlDumper;
+use Twig\Profiler\Profile;
+use Twig\Source;


@keradus just wondering: this useis not used in the class - is it a php-cs-fixer bug that fabbot is still green?

The NoUnusedImportsFixer is quite permissive (not removing an unsed import in better than removing an used one) particularly it check if the class in not referenced in a comment.

In this case it think it's the "source" word in the license header comment that make the fixer think that the class is still used.

that's exactly the case here.
Imported class name could be used in comment, even in different casing, which later is utilized by Doctrine or some annotation-parsers. that behaviour was added after fixer broke some code.
I would not focus here is it header comment or other comment actually.

One could bring configuration to fixer to ignore comments or non-docblock comments / be strict about casing !

Is there a way to make it more strict so that I can cleanup locally?

@nicolas-grekas Not currently, see PHP-CS-Fixer/PHP-CS-Fixer#2814.

go ahead @nicolas-grekas and contribute ;)

fabpot · 2017-05-30T13:47:37Z

src/Symfony/Bridge/Twig/Command/DebugCommand.php

+     * @param Environment $twig
     */
-    public function setTwigEnvironment(\Twig_Environment $twig)
+    public function setTwigEnvironment(Environment $twig)


Reading this, I would rename Twig\Environment to Twig\Twig

Is it possible to name it just Twig to avoid the Twig\Twig repetition?

Yep but you will have to import Twig\Twig anyway

We cannot use Twig only as all classes need to be under a namespace. That's PSR-4.

@javiereguiluz PSR-4 doesn't allow top-level classes. In §2.2.1, "The fully qualified class name MUST have a top-level namespace name, also known as a “vendor namespace”.
http://www.php-fig.org/psr/psr-4/#specification

renamed to Twig\Twig

nicolas-grekas · 2017-05-31T12:41:16Z

Just fixed a few potential BC breaks related to the use of aliases type hints.
Should be ready, waiting for twigphp/Twig#2490

GromNaN · 2017-05-31T14:45:45Z

src/Symfony/Bridge/Twig/Command/DebugCommand.php

                }

-                return !$param->getClass() || $param->getClass()->getName() !== 'Twig_Environment';
+                return !$param->getClass() || !in_array($param->getClass()->getName(), array('Twig_Environment', 'Twig\Twig'));


Using class_alias, the ReflectionClass refers to the original class. Checking against Twig\Twig is not necessary.
https://3v4l.org/QH2KQ

True, until Twig moves to namespaces-first, which is why this code is here.

Actually, this logic could be improved to avoid relying on typehints or param names (btw, the check on param name is wrong, as the name is not relevant). I will send a PR.

here you go: #22994

@nicolas-grekas please revert this change to avoid useless conflicts with my PR removing this code entirely 😄

reverted, thanks

stof · 2017-05-31T15:28:00Z

src/Symfony/Bridge/Twig/Extension/RoutingExtension.php

     *
     * @return array An array with the contexts the URL is safe
+     *
+     * To be made @final in 3.4, and the type-hint be changed to "\Twig\Node\Node" in 4.0.


Wouldn't this trigger the warning already ? Or does DebugClassLoader check for beginning of lines ?

it's ok: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Debug/DebugClassLoader.php#L171

stof · 2017-05-31T15:29:24Z

src/Symfony/Bridge/Twig/Extension/RoutingExtension.php

+     *
+     * To be made @final in 3.4, and the type-hint be changed to "\Twig\Node\Node" in 4.0.
     */
    public function isUrlGenerationSafe(\Twig_Node $argsNode)


Can't we already change the typehint here (with the same class_exists call) ?

We can, but that means loading Node, which in turn loads Compiler.
I preferred not, thus the current code.

nicolas-grekas · 2017-05-31T19:42:06Z

now green

fabpot

You just need to revert the Twig\Environment => Twig\Twig change. Sorry about that :)

nicolas-grekas · 2017-06-01T22:05:42Z

renaming done, now green

This PR was merged into the 2.7 branch. Discussion ---------- Use namespaced Twig | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | needs twigphp/Twig#2490 and twigphp/Twig#2491 | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- c3d1262 Use namespaced Twig

nicolas-grekas added this to the 2.7 milestone May 30, 2017

carsonbot added Status: Needs Review Bug labels May 30, 2017

nicolas-grekas commented May 30, 2017

View reviewed changes

nicolas-grekas mentioned this pull request May 30, 2017

[1.x] Allow using namespaced aliases as extension names twigphp/Twig#2490

Merged

fabpot requested changes May 30, 2017

View reviewed changes

nicolas-grekas force-pushed the twig branch from 26d67ea to d4059db Compare May 30, 2017 16:10

julienfalque mentioned this pull request May 30, 2017

NoUnusedImportsFixer - Ignore references in comments PHP-CS-Fixer/PHP-CS-Fixer#2814

Closed

nicolas-grekas force-pushed the twig branch 4 times, most recently from 6a2452b to 4cfb449 Compare May 31, 2017 12:33

GromNaN reviewed May 31, 2017

View reviewed changes

stof reviewed May 31, 2017

View reviewed changes

nicolas-grekas force-pushed the twig branch 3 times, most recently from 223b126 to 029377d Compare May 31, 2017 19:32

fabpot approved these changes May 31, 2017

View reviewed changes

nicolas-grekas force-pushed the twig branch from 029377d to b505b62 Compare May 31, 2017 21:30

Use namespaced Twig

c3d1262

nicolas-grekas force-pushed the twig branch from b505b62 to c3d1262 Compare June 1, 2017 21:44

nicolas-grekas merged commit c3d1262 into symfony:2.7 Jun 2, 2017

nicolas-grekas deleted the twig branch June 2, 2017 08:18

weaverryan mentioned this pull request Jun 5, 2017

Accidental Twig Namespace break in 3.3.1 #23072

Closed

GwendolenLynch mentioned this pull request Jun 6, 2017

[3.4][Deprecations] Use namespaced Twig bolt/bolt#6742

Merged

Uh oh!

Conversation

nicolas-grekas commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvasseur May 30, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

GromNaN May 30, 2017 • 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!

nicolas-grekas commented May 31, 2017

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!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas May 31, 2017 • 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!

nicolas-grekas commented May 31, 2017

Uh oh!

fabpot left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jun 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jvasseur May 30, 2017 •

edited

Loading

GromNaN May 30, 2017 •

edited

Loading

nicolas-grekas May 31, 2017 •

edited

Loading