Skip to content
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

[RFC] Revert the move of translations/ into config/ #253

Merged
1 commit merged into from Nov 21, 2017

Conversation

javiereguiluz
Copy link
Member

Q A
License MIT

I propose to revert the changes done in #249. These are my reasons for doing so ... but of course there could be strong reasons too to not do it.


We've been using translations/ for months ... and two days ago it was suddenly changed to config/translations/. There wasn't a public discussion about it. Not even a single comment in #249. The proposal was made at 18:12 CET and merged at 18:50 CET. Just 38 minutes!!


The full dir structure of a modern Symfony project is:

assets/
bin/
config/
public/
src/
templates/
tests/
translations/
var/
vendor/

Removing translations/ won't make it significantly more compact. However, hiding it inside config/ has a serious drawback: instead of being self-explanatory ("translations are stored in the translations/ dir. done!") you need to explain things: "translations are stored inside the config/ dir"


Related to the previous point, it's very debatable to consider translations as part of the config. Some people do, others don't. In fact, the way translations are often used in Symfony, they are part of templates ... so they should be stored in templates/translations/ instead of config/


Historically, translations were never part of Symfony's config (app/config/) but of Symfony's resources (app/Resources/translations/).


Discoverability of this directory is important because it's one of those dirs that could be browsed by non backend developers (such as assets/ and templates/).

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@stof
Copy link
Member

stof commented Nov 13, 2017

@fabpot looking at the Symfony PR linked in #249, it looks like you were the one requesting this move. So can you give your mind here ?

@javiereguiluz the quick merge is related to the fact that a bot is merging PRs for this repo, so it merges as soon as it gets the required approvals.

@chalasr
Copy link
Member

chalasr commented Nov 15, 2017

Not having directories that depend on the project scope/dependencies at the project root level seems better to me.
You won't have a translations/ dir if your project has no translations, same applies to templates. Thus, a SF4 project's root folder structure would be different from a project to another, that does not feel right IMHO, even if it is simpler to teach (but I'm not convinced it is).

@ro0NL
Copy link
Contributor

ro0NL commented Nov 18, 2017

did we ever consider having both config/ and etc/, favoring etc/translations over config/translations? I agree with @chalasr keeping standardized project root dirs at a minimum feels better.

@javiereguiluz
Copy link
Member Author

@ro0NL but there are at least 3 directories whose contents are not only managed by developers: assets, templates and translations. We have <project>/assets/ and <project>/templates/ ... it's strange to have <project>/config/translations/ instead of <project>/translations/

@ro0NL
Copy link
Contributor

ro0NL commented Nov 18, 2017

agree that's why i think we should consider having both config/ + etc/.

  • /etc/translations
  • /etc/templates
  • /etc/assets/
  • /etc/whatever

@javiereguiluz
Copy link
Member Author

javiereguiluz commented Nov 18, 2017

I totally disagree because of "discoverability" and "simplicity". These days we've updated tens of Symfony Docs for Symfony 4. The best thing about that was that lots of things no longer need to be explained: they are self-explanatory and that's great.

If templates are stored in <project>/templates/ you don't have to explain anything. Even someone with zero previous Symfony experience will understand that instantly and without explaining it.

If templates are stored in <project>/etc/templates/ you need to explain lots of things:

  • templates are stored inside etc/ ... memorize that!
  • and be careful, etc/ is used for config in computer industry, but in Symfony apps we store the config in config/ not etc/
  • and etc/ is used for lots of different and incoherent things ... but not for resources, we have src/Resources/ for that

@ogizanagi
Copy link
Member

ogizanagi commented Nov 18, 2017

Anyway about /etc, we already gave it a try but it was reverted, listening to the community as it wasn't obvious what goes in for everyone.

I admit config isn't really the best place for translations nor just moving the /translations dir from root would make it more compact...

The original issue was actually asking how to replace the old app/Resources/ folder, but there wasn't really any discussion. However, a root /res directory (or /resources if you really want it more self-explanatory) was mentioned by multiple people in symfony/symfony-standard#1034.
Gathering resources needing to be located and consumed by your application, it'll allow to unclutter much more the root directory, by putting translations, assets, templates or even migrations and fixtures within.

Projects root dirs are already filled with a lot of directories and (.dot)files. I find this overwhelming for everyone. If we can reduce this and the disparity between projects using or not templating, translations, etc... I think we should.


In any case, I agree we've done this change too fast, without any real discussion nor giving enough time for the community to speak. I'm sorry about that, I should have opened an issue first after the merge of the default path for translations. But if we revert this commit, I guess we should also update the default value for this new config option.

@ro0NL
Copy link
Contributor

ro0NL commented Nov 18, 2017

and etc/ is used for lots of different and incoherent things ... but not for resources, we have src/Resources/ for that

Im not sure src/Resources and etc/ serve the same purpose. I wouldnt necessarily expect project tied resources to be found under src/. IMHO src/Resources would contain things required/leveraged by source code relative to ../ (i.e. src/).

Projects root dirs are already filled with a lot of directories and (.dot)files.

Agree, claiming all root directories could make the project dir look dramatically huge. Not necessarily making it more easy to discover, as other "project related" things might root here as well.

Anyway about /etc, we already gave it a try but it was reverted

I suggest to use both directories :) Given we cant put everything inside config/ nor inside the project root dir. etc/ === app/Resources to me, a clear upgrade path also.

@javiereguiluz
Copy link
Member Author

I don't think project root dirs are crowded. And if they are, we should work to reduce the useless dot files, not the real project directories.

@fabpot
Copy link
Member

fabpot commented Nov 21, 2017

Ok, after thinking a bit more about this one, I think @javiereguiluz is right. Let's revert.

@ro0NL
Copy link
Contributor

ro0NL commented Nov 21, 2017

👍 for revert. Still not sure we should claim all project root dirs (/something vs. /etc/something, in case /config/something/ or src/Resources/something dont fit).

Today i would do /app/something (not a root/src/config folder). Then again the user can change dirs as needed, so no real issues anyway :)

@ogizanagi
Copy link
Member

Ok for revert. Still not convinced this should be at the root though.

we should work to reduce the useless dot files

We cannot reduce the nb of dot files used in projects, we don't have the hand on it as these are required by third-party tools.

@yceruto
Copy link
Member

yceruto commented Nov 21, 2017

Updating default path in symfony/symfony#25083

nicolas-grekas added a commit to symfony/symfony that referenced this pull request Nov 21, 2017
…uto)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle] Update default translations path

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/recipes#253 (comment)
| License       | MIT
| Doc PR        | -

Moving default `translations/` dir to the root project again.

Commits
-------

ba3476c Update default translations path
@ghost ghost merged commit 18d744a into symfony:master Nov 21, 2017
ghost pushed a commit that referenced this pull request Nov 21, 2017
javiereguiluz added a commit to symfony/demo that referenced this pull request Nov 24, 2017
This PR was squashed before being merged into the master branch (closes #703).

Discussion
----------

Moved translations back to the project root

This was approved: symfony/recipes#253 so translations must be moved back to the project root dir.

Commits
-------

ec60dc2 Moved translations back to the project root
sayjun0505 added a commit to sayjun0505/sym_proj that referenced this pull request Apr 16, 2023
This PR was squashed before being merged into the master branch (closes #703).

Discussion
----------

Moved translations back to the project root

This was approved: symfony/recipes#253 so translations must be moved back to the project root dir.

Commits
-------

ec60dc2 Moved translations back to the project root
mwhorse46 added a commit to mwhorse46/sym_proj that referenced this pull request Apr 16, 2023
This PR was squashed before being merged into the master branch (closes #703).

Discussion
----------

Moved translations back to the project root

This was approved: symfony/recipes#253 so translations must be moved back to the project root dir.

Commits
-------

ec60dc2 Moved translations back to the project root
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants