Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[TwigBridge] Fix missing path and separators in loader paths list on debug:twig output#27728
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
(Travis failures unrelated) |
| { | ||
| $tester = $this->createCommandTester(array( | ||
| 'Acme' => array('extractor', 'extractor'), | ||
| '!Acme' => array('extractor', 'extractor'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suggest using different paths for some of them, to make the test a bit more realistic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Right, but that would require create an empty directory (with.gitignore file) intoFixtures (root path), anyway they are fakes paths to show the line separator, it still worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added a comment to avoid any confusion when reading the code.
ed1ef7b to553066aCompareI 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? |
@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 tablewhen 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). |
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:
This is how it'd look: Before After |
yceruto commentedJun 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Javier about:
Look good to me. |
Any update about this? We are all good with the empty lines as namespace separator? thanks. |
4336f45 toba7a4caCompareThank 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 || --- | --- ||  |  |**Extra Lines separator:**| Before | After || --- | --- ||  |  |Commits-------ba7a4ca [TwigBridge] Fix missing path and separators in loader paths list on debug:twig output
Uh oh!
There was an error while loading.Please reload this page.
Reproducer
See "Loader Paths" section at the end of the output and compare it with
--format=json. Note that the roottemplatespath is present in Twigloader_pathsbecausetwig.default_pathconfig, but it isn't visible ondebug:twigoutput.Missing path:
Extra Lines separator: