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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TwigBridge] Show Twig's loader paths on debug:twig command #24064

Closed
wants to merge 3 commits into
base: 3.4
from

Conversation

Projects
None yet
8 participants
@yceruto
Contributor

yceruto commented Sep 1, 2017

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

bin/console debug:twig:
twig-loader-paths

This information is not displayed anywhere ATM and it should be important to know:

  • The Twig's namespaces availables
  • The Twig's paths availables
  • The order that templates will be loaded ( regarding its namespace -> LOAD PRIORITY ! )

So it should help us to debug any issue related to circular templates reference, invalid namespaces, unsuccessful attempt to override a template, etc.

WDYT?

@chalasr chalasr added the TwigBridge label Sep 1, 2017

@yceruto yceruto changed the title from Show Twig's loader paths on debug:twig command to [TwigBridge] Show Twig's loader paths on debug:twig command Sep 1, 2017

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 1, 2017

Contributor

I have another outputs for this list, which do you prefer?

  • (A): @namespace: path (it looks more like how one use it @) (current PR)
  • (B): path: namespace (it looks more like how one define it in twig.paths configuration, yaml's style)
  • (C): path: @namespace (it looks like both A & B together)
Contributor

yceruto commented Sep 1, 2017

I have another outputs for this list, which do you prefer?

  • (A): @namespace: path (it looks more like how one use it @) (current PR)
  • (B): path: namespace (it looks more like how one define it in twig.paths configuration, yaml's style)
  • (C): path: @namespace (it looks like both A & B together)
@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 1, 2017

Member

For the record, I prefer the current version (A).

Member

ogizanagi commented Sep 1, 2017

For the record, I prefer the current version (A).

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 1, 2017

Member

Outputting namespace first looks better to me (so A), as what is important is the different paths related to a given namespace. Paths used for separate namespaces are irrelevant regarding their order

Member

stof commented Sep 1, 2017

Outputting namespace first looks better to me (so A), as what is important is the different paths related to a given namespace. Paths used for separate namespaces are irrelevant regarding their order

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 1, 2017

Contributor

Updated screenshot in description with the latest changes.

Contributor

yceruto commented Sep 1, 2017

Updated screenshot in description with the latest changes.

@fabpot

fabpot approved these changes Sep 1, 2017

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 1, 2017

Member

I find using * before each path and TableSeparator between each namespace really cluttering. :/

Which of the following outputs would you prefer?

  1. Asterisk bullets, with separator (current)screenshot 2017-09-01 a 21 46 11
  2. No bullet, with separator screenshot 2017-09-01 a 21 46 31
  3. Asterisk bullets, no separator screenshot 2017-09-01 a 21 46 52
  4. Dash bullets, no separator screenshot 2017-09-01 a 21 47 07
  5. No bullet, no separator screenshot 2017-09-01 a 21 47 32
  6. Dash bullets with blank separator (as proposed below)screenshot 2017-09-01 a 21 47 32
  7. Dash bullets with blank separator only if needed (as proposed below)screenshot 2017-09-01 a 22 42 54

My own vote would go to 4 or 5 (but 7's not bad, that's true 😄).

Member

ogizanagi commented Sep 1, 2017

I find using * before each path and TableSeparator between each namespace really cluttering. :/

Which of the following outputs would you prefer?

  1. Asterisk bullets, with separator (current)screenshot 2017-09-01 a 21 46 11
  2. No bullet, with separator screenshot 2017-09-01 a 21 46 31
  3. Asterisk bullets, no separator screenshot 2017-09-01 a 21 46 52
  4. Dash bullets, no separator screenshot 2017-09-01 a 21 47 07
  5. No bullet, no separator screenshot 2017-09-01 a 21 47 32
  6. Dash bullets with blank separator (as proposed below)screenshot 2017-09-01 a 21 47 32
  7. Dash bullets with blank separator only if needed (as proposed below)screenshot 2017-09-01 a 22 42 54

My own vote would go to 4 or 5 (but 7's not bad, that's true 😄).

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 1, 2017

Contributor

and 6 :) (Dash bullets with blank separator) as proposed above :)

  1. Dash bullets with blank separator only if needed.
    7
Contributor

yceruto commented Sep 1, 2017

and 6 :) (Dash bullets with blank separator) as proposed above :)

  1. Dash bullets with blank separator only if needed.
    7
@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 1, 2017

Contributor

The problem I see with no row separator is that there is no grouping view per namespace, so one could get confused (missing namespace 😕)

Contributor

yceruto commented Sep 1, 2017

The problem I see with no row separator is that there is no grouping view per namespace, so one could get confused (missing namespace 😕)

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 1, 2017

Contributor

My vote is for 7.

Contributor

yceruto commented Sep 1, 2017

My vote is for 7.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 1, 2017

Contributor

What about a --layout=[1-7] option :) And definitely support for layout=random 😂

7 is nice, agree with @yceruto; you getter a better view of path inheritance per namespace.

Tend to prefer (Main) / (Root) over (None) and have it on top.

edit: on 2nd thought (None) is fine :)

Contributor

ro0NL commented Sep 1, 2017

What about a --layout=[1-7] option :) And definitely support for layout=random 😂

7 is nice, agree with @yceruto; you getter a better view of path inheritance per namespace.

Tend to prefer (Main) / (Root) over (None) and have it on top.

edit: on 2nd thought (None) is fine :)

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Sep 4, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 5, 2017

Member

Thank you @yceruto.

Member

fabpot commented Sep 5, 2017

Thank you @yceruto.

fabpot added a commit that referenced this pull request Sep 5, 2017

feature #24064 [TwigBridge] Show Twig's loader paths on debug:twig co…
…mmand (yceruto)

This PR was squashed before being merged into the 3.4 branch (closes #24064).

Discussion
----------

[TwigBridge] Show Twig's loader paths on debug:twig command

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

`bin/console debug:twig`:
![twig-loader-paths](https://user-images.githubusercontent.com/2028198/29986784-ee5a4094-8f32-11e7-86c8-c78183630221.png
)

This information is not displayed anywhere ATM and it should be important to know:
 * The Twig's namespaces availables
 * The Twig's paths availables
 * The order that templates will be loaded ( regarding its namespace -> LOAD PRIORITY ! )

So it should help us to debug any issue related to circular templates reference, invalid namespaces, unsuccessful attempt to override a template, etc.

WDYT?

Commits
-------

3fdcb40 [TwigBridge] Show Twig's loader paths on debug:twig command

@fabpot fabpot closed this Sep 5, 2017

@yceruto yceruto deleted the yceruto:twig_loader_paths branch Sep 5, 2017

This was referenced Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment