Skip to content

Some Symfony templates are not respecting the "Symfony twig naming standard" #54051

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

Closed
VincentLanglet opened this issue Feb 25, 2024 · 3 comments

Comments

@VincentLanglet
Copy link
Contributor

Symfony version(s) affected

all

Description

When looking at Symfony standard about template naming
(https://symfony.com/doc/current/templates.html#template-naming or https://symfony.com/doc/current/best_practices.html#use-snake-case-for-template-names-and-variables)
it's said:

Use snake case for filenames and directories (e.g. blog_posts.html.twig, admin/default_theme/blog/index.html.twig, etc.);

Now, If we look at the UnicodeString::snake() in order to knows what a valid snake case is:
https://github.com/symfony/string/blob/7.0/Tests/AbstractAsciiTestCase.php#L1063-L1077
it seems that foo_2 is not a valid snake case and should be foo2.

But, when looking at symfony code
https://github.com/symfony/symfony/tree/7.1/src/Symfony/Bridge/Twig/Resources/views/Form
all the templates with number are in the format foo_2 instead of foo2
and therefore are not respecting the own Symfony templates naming standard.

How to reproduce

Irrelevant

Possible Solution

I see four solutions:

  1. Updating the naming standard to explain the "snake_case" strategy prefix all numbers a with an underscore
    => Certainly the simplest but less satisfying solution because it kinda introduce a new naming strategy witch is not even supported by Symfony/string.

  2. Changing the snake method strategy
    => Seems this is a BC break, this seem impossible

  3. Allow passing a new parameters to snake method/introduce a new method related to this snake_case_numeric strategy
    => Since [String] Introduce underscore() method #34781 was closed, not sure it will be preferred now (@simPod & @nicolas-grekas)

  4. Renaming the templates with numbers in a BC way.
    => Might be the best solution, it just need a little work to avoid BC break.

Additional Context

No response

@smnandre
Copy link
Member

smnandre commented May 2, 2024

I'm thinking this is a "choice" in String and not something we should base the rule on to consider a file in "snake case".

In my mind, there is how snake case "should" behave

  • everything not alnum is replaced with a _
  • multiple _ are reduced to one
  • remove trailing and ending _
version "my" snake String snake
1.0.0 1_0_0 100
10.0 10_0 100
100 100 100

... but that would be a massive BC .... so impossible.

Maybe use (in the Rule) a simple regex like [:alnum:]+(_[:alnum:]+)* ?

@wouterj
Copy link
Member

wouterj commented May 2, 2024

I would say this is not worth any BC break or deprecation for such a minor detail.

As you say both ways are used as "snake case" and for this reason I think any CS checker should also approve both as following the "use snake case" guideline (so no changes needed in the documentation). For the String component, which has to construct a snake case string, it must choose one of them and it's not worth changing existing behavior.

@VincentLanglet
Copy link
Contributor Author

Thanks for the feedback.
I changed my implementation then VincentLanglet/Twig-CS-Fixer#222

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

No branches or pull requests

4 participants