Movatterモバイル変換

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

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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

Lines 170 to 172 in6bbb5bc

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

Use the%twig.default_path% parameter instead.

Copy link

Member

ycerutoApr 13, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Author

Choose a reason for hiding this comment

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

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?

Copy link

Author

Choose a reason for hiding this comment

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

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?

Copy link

Member

ycerutoApr 15, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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 byTwigBundle:

(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?

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

Copy link

Author

pmontoyaApr 15, 2018

Choose a reason for hiding this comment

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

Done !

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 the4.1 milestone

Apr 14, 2018

pmontoya force-pushed thefeature-26898 branch 6 times, most recently froma3159d4 to366d97aCompare

April 16, 2018 05:08

fmata suggested changes

Apr 16, 2018

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

		private$rootDir;

		publicfunction__construct(Environment$twig,string$projectDir =null)
		publicfunction__construct(Environment$twig,string$projectDir =null,array$bundlesMetadata =array(),string$twigDefaultPath,string$rootDir)

Copy link

Contributor

fmataApr 16, 2018

Choose a reason for hiding this comment

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

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

Copy link

Author

pmontoyaApr 16, 2018

Choose a reason for hiding this comment

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

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

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Apr 16, 2018

pmontoya force-pushed thefeature-26898 branch 2 times, most recently from257aea3 to94629b8Compare

April 16, 2018 07:32

Copy link

Author

pmontoya commentedApr 16, 2018

Status: Needs Review

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

Copy link

Author

pmontoya commentedMay 2, 2018

Is something missing?

yceruto reviewed

May 2, 2018

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

		$data['tests'] =array_keys($data['tests']);
		$data['loader_paths'] =$this->getLoaderPaths();
		$io->writeln(json_encode($data));
		$this->displayAlternatives($this->findWrongBundleOverrides(),$io);

Copy link

Member

ycerutoMay 2, 2018

Choose a reason for hiding this comment

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

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

Copy link

Author

pmontoyaMay 3, 2018

Choose a reason for hiding this comment

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

Done. Thank you

pmontoya force-pushed thefeature-26898 branch 2 times, most recently from637598d to4ad9593Compare

May 3, 2018 15:41

xabbuh reviewed

src/Symfony/Bridge/Twig/CHANGELOG.md Outdated

		-----

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

Copy link

Member

xabbuhMay 4, 2018

Choose a reason for hiding this comment

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

overridden

xabbuh reviewed

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

		$data['tests'] =array_keys($data['tests']);
		$data['loader_paths'] =$this->getLoaderPaths();
		$wrongBundles =$this->findWrongBundleOverrides();
		if ($wrongBundles) {

Copy link

Member

xabbuhMay 4, 2018

Choose a reason for hiding this comment

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

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

xabbuh reviewed

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

		if ($wrongBundles) {
		$data['warnings'] =array();

		$messages =$this->buildWarningMessages($this->findWrongBundleOverrides());

Copy link

Member

xabbuhMay 4, 2018

Choose a reason for hiding this comment

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

Use$wrongBundles here?

xabbuh reviewed

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

		$data['loader_paths'] =$this->getLoaderPaths();
		$wrongBundles =$this->findWrongBundleOverrides();
		if ($wrongBundles) {
		$data['warnings'] =array();

Copy link

Member

xabbuhMay 4, 2018

Choose a reason for hiding this comment

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

this var is overridden three lines below

xabbuh reviewed

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

		}

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

Copy link

Member

xabbuhMay 4, 2018

Choose a reason for hiding this comment

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

I don't see why we need that variable

Copy link

Author

pmontoyaMay 14, 2018

Choose a reason for hiding this comment

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

Thanks@xabbuh Code updated

pmontoya force-pushed thefeature-26898 branch 2 times, most recently fromd5bd6ec to54f66a7Compare

May 14, 2018 10:07

Copy link

Author

pmontoya commentedJun 5, 2018•
edited
Loading

Something to add?

xabbuh reviewed

Jun 12, 2018

src/Symfony/Bridge/Twig/CHANGELOG.md Outdated

		-----

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

Copy link

Member

xabbuhJun 12, 2018

Choose a reason for hiding this comment

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

this needs to be an item under a to be added4.2.0 section

Copy link

Author

pmontoyaJun 13, 2018

Choose a reason for hiding this comment

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

Updated. Thank you

xabbuh reviewed

Jun 12, 2018

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

		$alternatives =array();
		$bundleNames =array();

		if ($this->rootDir) {

Copy link

Member

xabbuhJun 12, 2018

Choose a reason for hiding this comment

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

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

Copy link

Author

pmontoyaJun 13, 2018

Choose a reason for hiding this comment

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

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

pmontoya force-pushed thefeature-26898 branch 2 times, most recently from9d534ee toc687afcCompare

June 13, 2018 12:23

xabbuh reviewed

Jun 18, 2018

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

		$data['loader_paths'] =$this->getLoaderPaths();
		if ($wrongBundles =$this->findWrongBundleOverrides()) {
		$messages =$this->buildWarningMessages($wrongBundles);
		$data['warnings'] =array_reduce(

Copy link

Member

xabbuhJun 18, 2018

Choose a reason for hiding this comment

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

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

Copy link

Author

pmontoyaJun 18, 2018

Choose a reason for hiding this comment

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

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

xabbuh reviewed

Jun 18, 2018

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

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

Copy link

Member

xabbuhJun 18, 2018

Choose a reason for hiding this comment

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

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

Copy link

Author

pmontoyaJun 18, 2018

Choose a reason for hiding this comment

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

Done

Pascal Montoyaand others added6 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

refssymfony#26898

da0c589

Move changelog entry to 4.1.0 to 4.2.0

refssymfony#26898

da42b3e

Check if projectDir is not null before loop

refssymfony#26898

acfb325

Remove unnecessary loopTrim messages on build rather than on display

pmontoya force-pushed thefeature-26898 branch fromc687afc toacfb325Compare

June 18, 2018 10:28

carsonbot removed the Status: Needs Review label

xabbuh approved these changes

carsonbot added the Status: Reviewed label

nicolas-grekas approved these changes

Copy link

Member

nicolas-grekas commentedJun 19, 2018

Thank you@pmontoya.

nicolas-grekas merged commitacfb325 intosymfony:master

nicolas-grekas added a commit that referenced this pull request