[TwigBridge] Fix missing path and separators in loader paths list on debug:twig output #27728

yceruto · 2018-06-26T11:47:28Z

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

Reproducer

$ composer create-project symfony/skeleton repr
$ cd repr
$ composer req twig
# remove duplicated path on `twig.paths: ['%kernel.project_dir%/templates']` config.
$ bin/console debug:twig

See "Loader Paths" section at the end of the output and compare it with --format=json. Note that the root templates path is present in Twig loader_paths because twig.default_path config, but it isn't visible on debug:twig output.

Missing path:

Before	After

Extra Lines separator:

Before	After

yceruto · 2018-06-26T13:15:39Z

(Travis failures unrelated)

stof · 2018-06-26T13:19:24Z

src/Symfony/Bridge/Twig/Tests/Command/DebugCommandTest.php

+    {
+        $tester = $this->createCommandTester(array(
+            'Acme' => array('extractor', 'extractor'),
+            '!Acme' => array('extractor', 'extractor'),


I suggest using different paths for some of them, to make the test a bit more realistic

Right, but that would require create an empty directory (with .gitignore file) into Fixtures (root path), anyway they are fakes paths to show the line separator, it still worth it?

I added a comment to avoid any confusion when reading the code.

fabpot · 2018-06-27T07:20:56Z

I have a hard time (and I know that this is not really related to this PR) figuring out why/when we have empty lines. Can someone explain that to me?

yceruto · 2018-06-27T11:48:09Z

@fabpot empty lines (or namespace separator) was proposed in the original PR #24064 (comment) to solve some kind of cluttering in the original design of the table when there is more than one path per namespace #24064 (comment), but still we can reopen that discussion if you prefer remove these empty lines or another variant from #24064 (comment).

javiereguiluz · 2018-06-28T15:09:32Z

I don't mind the new lines (I think it makes this more readable) but I'd like to propose two tweaks for your consideration:

Remove the leading - before each path
Add a trailing / to all paths to clearly show that they are directories.

This is how it'd look:

Before

 --------------- ------------------------------------------------------
  Namespace       Paths
 --------------- ------------------------------------------------------
  @Framework      - vendor/symfony/framework-bundle/Resources/views
  @!Framework     - vendor/symfony/framework-bundle/Resources/views
  @Security       - vendor/symfony/security-bundle/Resources/views
  @!Security      - vendor/symfony/security-bundle/Resources/views
  ...             ...
 --------------- ------------------------------------------------------

After

 --------------- ------------------------------------------------------
  Namespace       Paths
 --------------- ------------------------------------------------------
  @Framework      vendor/symfony/framework-bundle/Resources/views/
  @!Framework     vendor/symfony/framework-bundle/Resources/views/
  @Security       vendor/symfony/security-bundle/Resources/views/
  @!Security      vendor/symfony/security-bundle/Resources/views/
  ...             ...
 --------------- ------------------------------------------------------

yceruto · 2018-06-28T16:32:28Z

Javier about:

Remove the leading - before each path

Add a trailing / to all paths to clearly show that they are directories.

Look good to me.

yceruto · 2018-07-07T12:26:51Z

Any update about this? We are all good with the empty lines as namespace separator? thanks.

…debug:twig output

nicolas-grekas · 2018-07-07T15:09:02Z

Thank you @yceruto.

…hs list on debug:twig output (yceruto) This PR was squashed before being merged into the 3.4 branch (closes #27728). Discussion ---------- [TwigBridge] Fix missing path and separators in loader paths list on debug:twig output | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - **Reproducer** ```bash $ composer create-project symfony/skeleton repr $ cd repr $ composer req twig # remove duplicated path on `twig.paths: ['%kernel.project_dir%/templates']` config. $ bin/console debug:twig ``` See "Loader Paths" section at the end of the output and compare it with `--format=json`. Note that the root `templates` path is present in Twig `loader_paths` because `twig.default_path` config, but it isn't visible on `debug:twig` output. **Missing path:** | Before | After | | --- | --- | | ![debug_twig_before1](https://user-images.githubusercontent.com/2028198/41908937-9af78274-7913-11e8-91d1-bb48f81031dd.png) | ![debug_twig_after1](https://user-images.githubusercontent.com/2028198/41908969-b5fd3140-7913-11e8-8129-8215f3c09b24.png) | **Extra Lines separator:** | Before | After | | --- | --- | | ![debug_twig_before2](https://user-images.githubusercontent.com/2028198/41909029-e867fcdc-7913-11e8-91f1-bba0d873495c.png) | ![debug_twig_after2](https://user-images.githubusercontent.com/2028198/41915586-12a6426e-7924-11e8-899f-9ecc7d95b74f.png) | Commits ------- ba7a4ca [TwigBridge] Fix missing path and separators in loader paths list on debug:twig output

carsonbot added Status: Needs Review TwigBridge Bug labels Jun 26, 2018

yceruto force-pushed the debug_twig_command branch from cf75c41 to 3f01dac Compare June 26, 2018 12:23

stof reviewed Jun 26, 2018

View reviewed changes

yceruto force-pushed the debug_twig_command branch 3 times, most recently from ed1ef7b to 553066a Compare June 26, 2018 13:56

nicolas-grekas added this to the 3.4 milestone Jun 26, 2018

nicolas-grekas mentioned this pull request Jun 26, 2018

[symfony/twig-bundle] remove "paths", thats already the default symfony/recipes#416

Closed

nicolas-grekas approved these changes Jun 26, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 26, 2018

yceruto force-pushed the debug_twig_command branch from 4b5e29d to 4336f45 Compare June 28, 2018 18:11

[TwigBridge] Fix missing path and separators in loader paths list on …

ba7a4ca

…debug:twig output

nicolas-grekas force-pushed the debug_twig_command branch from 4336f45 to ba7a4ca Compare July 7, 2018 15:04

nicolas-grekas merged commit ba7a4ca into symfony:3.4 Jul 7, 2018

yceruto deleted the debug_twig_command branch July 7, 2018 15:13

This was referenced Jul 23, 2018

Release v3.4.13 #28039

Merged

Release v4.0.13 #28042

Merged

Release v4.1.2 #28043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TwigBridge] Fix missing path and separators in loader paths list on debug:twig output #27728

[TwigBridge] Fix missing path and separators in loader paths list on debug:twig output #27728

yceruto commented Jun 26, 2018 •

edited

Loading

yceruto commented Jun 26, 2018

stof Jun 26, 2018

yceruto Jun 26, 2018 •

edited

Loading

yceruto Jun 26, 2018

fabpot commented Jun 27, 2018

yceruto commented Jun 27, 2018

javiereguiluz commented Jun 28, 2018

yceruto commented Jun 28, 2018 •

edited

Loading

yceruto commented Jul 7, 2018

nicolas-grekas commented Jul 7, 2018

[TwigBridge] Fix missing path and separators in loader paths list on debug:twig output #27728

[TwigBridge] Fix missing path and separators in loader paths list on debug:twig output #27728

Conversation

yceruto commented Jun 26, 2018 • edited Loading

yceruto commented Jun 26, 2018

stof Jun 26, 2018

Choose a reason for hiding this comment

yceruto Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

yceruto Jun 26, 2018

Choose a reason for hiding this comment

fabpot commented Jun 27, 2018

yceruto commented Jun 27, 2018

javiereguiluz commented Jun 28, 2018

yceruto commented Jun 28, 2018 • edited Loading

yceruto commented Jul 7, 2018

nicolas-grekas commented Jul 7, 2018

yceruto commented Jun 26, 2018 •

edited

Loading

yceruto Jun 26, 2018 •

edited

Loading

yceruto commented Jun 28, 2018 •

edited

Loading