[TwigBridge] Added bundle name suggestion on wrongly overrided templates paths #26919

pmontoya · 2018-04-13T14:23:10Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#26898
License	MIT

Report unknown folders in templates/bundles in debug:twig and make suggestions if any

Developped with @bricejulia

yceruto · 2018-04-13T15:00:55Z

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

+        $paths = array_unique($loaderPaths['(None)']);
+        $bundleNames = array();
+        foreach ($paths as $path) {
+            $relativePath = $path.'/bundles/';


You don't need to check all paths because only <twig.default_path>/bundles/... is taking into account:

symfony/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php

Lines 170 to 172 in 6bbb5bc

if (file_exists($dir = $container->getParameterBag()->resolveValue($config['default_path']).'/bundles/'.$name)) {

$bundleHierarchy[$name][] = $dir;

}

Use the %twig.default_path% parameter instead.

and <kernel.root_dir>/Resources/<BundleName>/views should be checked too?

I had the same question but has it's a 4.1 feature, I prefered to use the new Twig override convention. The previous one is no more present in official documentation. Do you think it's necessary?

For your first comment, I had the same idea but I did this choice for two reasons :

I don't check all paths, just generic ones

I prefered to use the result of other methods in the command that to duplicate code from another bundle.

Could you please tell me why is better to use the default path parameter?
Can someone give me another point of vue on it?

Sorry for my short explanation, I'll try to be more --vvv now :)

I don't check all paths, just generic ones

For instance, this is a generic one registered by TwigBundle:

(None) : vendor/symfony/twig-bridge/Resources/views/Form

and shouldn't be taken into account in this process because this path:

vendor/symfony/twig-bridge/Resources/views/Form/bundles/<BundleName>/

doesn't exists either as a Twig's path nor as a directory.

I prefered to use the result of other methods in the command that to duplicate code from another bundle.

My suggestion is about injecting the %twig.default_path% parameter to the command and check in one of the path convention to override templates: <twig.default_path>/bundles/<BundleName>. The another one <kernel.root_dir>/Resources/<BundleName>/views is not promoted anymore but still is available, IMO would be good have this feature here too, so someone using this path can still benefit from it.

Could you please tell me why is better to use the default path parameter?

Because TwigBundle has just two paths convention to override templates where you need looking for, hence isn't necessary other paths.

fmata · 2018-04-16T06:34:28Z

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


-    public function __construct(Environment $twig, string $projectDir = null)
+    public function __construct(Environment $twig, string $projectDir = null, array $bundlesMetadata = array(), string $twigDefaultPath, string $rootDir)


New parameters must have a default value to prevent a BC.

Sorry for forgetting. I wrote it in the first version and forget it in the second one. Thank you

pmontoya · 2018-04-16T16:07:38Z

Status: Needs Review

pmontoya · 2018-05-02T08:57:30Z

Is something missing?

yceruto · 2018-05-02T15:00:37Z

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

@@ -83,6 +89,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
            $data['tests'] = array_keys($data['tests']);
            $data['loader_paths'] = $this->getLoaderPaths();
            $io->writeln(json_encode($data));
+            $this->displayAlternatives($this->findWrongBundleOverrides(), $io);


It'll break the json output, maybe should be included into a new $data['warning'] above?

Done. Thank you

xabbuh · 2018-05-04T10:18:56Z

src/Symfony/Bridge/Twig/CHANGELOG.md

@@ -5,6 +5,7 @@ CHANGELOG
 -----

 * add a `workflow_metadata` function
+ * add bundle name suggestion on wrongly overrided templates paths


xabbuh · 2018-05-04T10:19:35Z

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

@@ -82,6 +88,22 @@ protected function execute(InputInterface $input, OutputInterface $output)
            }
            $data['tests'] = array_keys($data['tests']);
            $data['loader_paths'] = $this->getLoaderPaths();
+            $wrongBundles = $this->findWrongBundleOverrides();
+            if ($wrongBundles) {


if ($wrongBundles = $this->findWrongBundleOverrides()) {

xabbuh · 2018-05-04T10:21:01Z

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

+            if ($wrongBundles) {
+                $data['warnings'] = array();
+
+                $messages = $this->buildWarningMessages($this->findWrongBundleOverrides());


Use $wrongBundles here?

xabbuh · 2018-05-04T10:21:33Z

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

@@ -82,6 +88,22 @@ protected function execute(InputInterface $input, OutputInterface $output)
            }
            $data['tests'] = array_keys($data['tests']);
            $data['loader_paths'] = $this->getLoaderPaths();
+            $wrongBundles = $this->findWrongBundleOverrides();
+            if ($wrongBundles) {
+                $data['warnings'] = array();


this var is overridden three lines below

xabbuh · 2018-05-04T10:23:47Z

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

+        }
+
+        if (\count($bundleNames)) {
+            $loadedBundles = $this->bundlesMetadata;


I don't see why we need that variable

Thanks @xabbuh Code updated

pmontoya · 2018-06-05T14:13:28Z

Something to add?

xabbuh · 2018-06-12T08:21:40Z

src/Symfony/Bridge/Twig/CHANGELOG.md

@@ -5,6 +5,7 @@ CHANGELOG
 -----

 * add a `workflow_metadata` function
+ * add bundle name suggestion on wrongly overridden templates paths


this needs to be an item under a to be added 4.2.0 section

Updated. Thank you

xabbuh · 2018-06-12T08:28:05Z

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

+        $alternatives = array();
+        $bundleNames = array();
+
+        if ($this->rootDir) {


I think we need to account for $this->projectDir being null here too.

Done for this one and the next if too. Thank you

xabbuh · 2018-06-18T09:52:21Z

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

@@ -82,6 +88,19 @@ protected function execute(InputInterface $input, OutputInterface $output)
            }
            $data['tests'] = array_keys($data['tests']);
            $data['loader_paths'] = $this->getLoaderPaths();
+            if ($wrongBundles = $this->findWrongBundleOverrides()) {
+                $messages = $this->buildWarningMessages($wrongBundles);
+                $data['warnings'] = array_reduce(


I am not sure I understand for what we need array_reduce() here. $messages already is just an array of strings, isn't it?

You're right. It was just dead code. Done

xabbuh · 2018-06-18T09:56:11Z

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

+        array_walk(
+            $messages,
+            function ($message) use ($io) {
+                $io->warning(trim($message));


Can't we just trim in buildWarningMessages() and then iterate the array here?

…tes paths

Move changelog entry to 4.1.0 to 4.2.0

Check if projectDir is not null before loop

Remove unnecessary loop Trim messages on build rather than on display

nicolas-grekas · 2018-06-19T12:50:38Z

Thank you @pmontoya.

@bricejulia

…verrided templates paths (pmontoya, Pascal Montoya) This PR was merged into the 4.2-dev branch. Discussion ---------- [TwigBridge] Added bundle name suggestion on wrongly overrided templates paths | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26898 | License | MIT Report unknown folders in templates/bundles in debug:twig and make suggestions if any Developped with @bricejulia Commits ------- acfb325 refs #26898 Remove unnecessary loop Trim messages on build rather than on display da42b3e refs #26898 Check if projectDir is not null before loop da0c589 refs #26898 Move changelog entry to 4.1.0 to 4.2.0 7d9467a Add an entry on json export format bab9d99 Use %twig.default_path% parameter and search in old folder structure too 1758de2 [TwigBridge] Added bundle name suggestion on wrongly overrided templates paths

carsonbot added Status: Needs Review TwigBridge Feature labels Apr 13, 2018

pmontoya force-pushed the feature-26898 branch from d9d6bf3 to 7480823 Compare April 13, 2018 14:26

yceruto reviewed Apr 13, 2018

View reviewed changes

nicolas-grekas changed the title ~~[TwigBridge] Added bundle name suggestion on wrongly overrided templa…~~ [TwigBridge] Added bundle name suggestion on wrongly overrided templates paths Apr 14, 2018

nicolas-grekas added this to the 4.1 milestone Apr 14, 2018

pmontoya force-pushed the feature-26898 branch 6 times, most recently from a3159d4 to 366d97a Compare April 16, 2018 05:08

fmata suggested changes Apr 16, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 16, 2018

pmontoya force-pushed the feature-26898 branch 2 times, most recently from 257aea3 to 94629b8 Compare April 16, 2018 07:32

carsonbot added Status: Needs Review and removed Status: Needs Work labels Apr 16, 2018

nicolas-grekas modified the milestones: 4.1, next May 1, 2018

yceruto reviewed May 2, 2018

View reviewed changes

pmontoya force-pushed the feature-26898 branch 2 times, most recently from 637598d to 4ad9593 Compare May 3, 2018 15:41

xabbuh reviewed May 4, 2018

View reviewed changes

pmontoya force-pushed the feature-26898 branch 2 times, most recently from d5bd6ec to 54f66a7 Compare May 14, 2018 10:07

xabbuh reviewed Jun 12, 2018

View reviewed changes

pmontoya force-pushed the feature-26898 branch 2 times, most recently from 9d534ee to c687afc Compare June 13, 2018 12:23

xabbuh reviewed Jun 18, 2018

View reviewed changes

Pascal Montoya and others added 6 commits June 18, 2018 12:27

[TwigBridge] Added bundle name suggestion on wrongly overrided templa…

1758de2

…tes paths

Use %twig.default_path% parameter and search in old folder structure too

bab9d99

Add an entry on json export format

7d9467a

refs symfony#26898

da0c589

Move changelog entry to 4.1.0 to 4.2.0

refs symfony#26898

da42b3e

Check if projectDir is not null before loop

refs symfony#26898

acfb325

Remove unnecessary loop Trim messages on build rather than on display

pmontoya force-pushed the feature-26898 branch from c687afc to acfb325 Compare June 18, 2018 10:28

carsonbot removed the Status: Needs Review label Jun 19, 2018

xabbuh approved these changes Jun 19, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Jun 19, 2018

nicolas-grekas approved these changes Jun 19, 2018

View reviewed changes

nicolas-grekas merged commit acfb325 into symfony:master Jun 19, 2018

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TwigBridge] Added bundle name suggestion on wrongly overrided templates paths #26919

[TwigBridge] Added bundle name suggestion on wrongly overrided templates paths #26919

pmontoya commented Apr 13, 2018 •

edited by nicolas-grekas

Loading

yceruto Apr 13, 2018

yceruto Apr 13, 2018 •

edited

Loading

pmontoya Apr 15, 2018

pmontoya Apr 15, 2018

yceruto Apr 15, 2018 •

edited

Loading

pmontoya Apr 15, 2018

fmata Apr 16, 2018

pmontoya Apr 16, 2018

pmontoya commented Apr 16, 2018

pmontoya commented May 2, 2018

yceruto May 2, 2018

pmontoya May 3, 2018

xabbuh May 4, 2018

xabbuh May 4, 2018

xabbuh May 4, 2018

xabbuh May 4, 2018

xabbuh May 4, 2018

pmontoya May 14, 2018

pmontoya commented Jun 5, 2018 •

edited

Loading

xabbuh Jun 12, 2018

pmontoya Jun 13, 2018

xabbuh Jun 12, 2018

pmontoya Jun 13, 2018

xabbuh Jun 18, 2018

pmontoya Jun 18, 2018

xabbuh Jun 18, 2018

pmontoya Jun 18, 2018

nicolas-grekas commented Jun 19, 2018

	if (file_exists($dir = $container->getParameterBag()->resolveValue($config['default_path']).'/bundles/'.$name)) {
	$bundleHierarchy[$name][] = $dir;
	}


		public function __construct(Environment $twig, string $projectDir = null)
		public function __construct(Environment $twig, string $projectDir = null, array $bundlesMetadata = array(), string $twigDefaultPath, string $rootDir)

[TwigBridge] Added bundle name suggestion on wrongly overrided templates paths #26919

[TwigBridge] Added bundle name suggestion on wrongly overrided templates paths #26919

Conversation

pmontoya commented Apr 13, 2018 • edited by nicolas-grekas Loading

Choose a reason for hiding this comment

yceruto Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmontoya commented Apr 16, 2018

pmontoya commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmontoya commented Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jun 19, 2018

pmontoya commented Apr 13, 2018 •

edited by nicolas-grekas

Loading

yceruto Apr 13, 2018 •

edited

Loading

yceruto Apr 15, 2018 •

edited

Loading

pmontoya commented Jun 5, 2018 •

edited

Loading