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

[FrameworkBundle][Translations] Fix translation commands #24590

Closed

Conversation

kevin-verschaeve
Copy link
Contributor

Hi,

I'm not sure about the target branch, but this bug seems to be present since we have the new translations directory in the project root.

This add the search for a translations directory in the project root, and enable to have translations with .yaml extension, for now only .yml is working.

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

Please tell me how can I test this.

$transPaths = array($kernel->getRootDir().'/Resources/');
$transPaths = array(
$kernel->getRootDir().'/Resources/',
$kernel->getProjectDir().'/',

Choose a reason for hiding this comment

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

There is one problem here. These paths are used to generate views and translations folders paths (see L275 & L295). With Flex, views is renamed by templates, so translations could be extracted but translation keys used in templates won't be extracted.

#24807 (comment)

Choose a reason for hiding this comment

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

I propose you a simple solution in PR.

@chalasr
Copy link
Member

chalasr commented Nov 3, 2017

Translation paths are already collected by framework extension. Couldn't we inject them in the command instead of discovering them twice?

@baptistedonaux
Copy link

@chalasr I think is a better solution but the current implementation has probably implemented for specific reason.

@chalasr
Copy link
Member

chalasr commented Nov 5, 2017

@baptistedonaux At time it has been introduced (#10368), a bundle argument was required by the command.
In #13443, %kernel.root_dir%/Resources/translations was added as fallback directory (), still letting only 2 possible dirs, both available via existing parameters. So it was just not worth it.

Later, a paths option was added to the framework.translator config node (#14561).
Currently these paths are not taken into account by translation commands.
I think we should fill this gap on 3.4 where these commands are registered as services and have constructors. I will take care of the PR.

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

👍

@@ -33,7 +33,7 @@
</service>

<service id="translation.loader.yml" class="Symfony\Component\Translation\Loader\YamlFileLoader" public="true">
<tag name="translation.loader" alias="yml" />
<tag name="translation.loader" alias="yaml" legacy-alias="yml" />
Copy link
Member

Choose a reason for hiding this comment

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

hmm, is that another bugfix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. It allow to find .yml and .yaml files for translations. I let this in this PR because I think it is part of the command fix

@kevin-verschaeve
Copy link
Contributor Author

Hi, thanks for your review.

So what needs to be done ? Should I update the code to get the paths from the path options of the translator ?

@baptistedonaux
Copy link

baptistedonaux commented Nov 6, 2017

Probably the best way. @chalasr , your opinion ?

@chalasr
Copy link
Member

chalasr commented Nov 6, 2017

From me this is OK regarding commands, it just looks into %kernel.project_dir%/translations as of 3.4, that seems to make sense. I will have a look to custom paths handling, it has to be done on another branch.

Search for translations also in the new translations dir at the project root
Search for .yaml file as well as .yml
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

For now the %kernel.project_dir%/translations/ dir has been abandoned in favor of (symfony/recipes#249) so this should be updated accordingly, but that's still in discussion in symfony/recipes#253. Let's wait

@kevin-verschaeve
Copy link
Contributor Author

I should close this PR in favor of #25065, seems to be more updated and better implemented

@nicolas-grekas
Copy link
Member

Looks like we kinda missed your PR... this would mean #25065 should be rebased on 3.3? @yceruto WDYT?

@yceruto
Copy link
Member

yceruto commented Nov 22, 2017

Hmm, not sure how right now ... I know 4.0 structure + flex is compatible since 3.3, but the solucion here is a little tricky. IMO 3.4 is a better place to migrate to the new structure as there we have the default paths and the new overriding conventions related to translations/ & templates/ directories and because they are new features I suggest to keep structure as is in 3.3 and then change it in 3.4 as a progressive migration path in our projects. Wdyt?

@chalasr
Copy link
Member

chalasr commented Nov 22, 2017

Supporting the new directory structure starting from 3.4 makes sense to me.

@nicolas-grekas
Copy link
Member

So, shall we close this PR?

@chalasr
Copy link
Member

chalasr commented Nov 23, 2017

Yea, closing in favor of #25065 which is more advanced.
Thank you for starting the work @kevin-verschaeve.

@chalasr chalasr closed this Nov 23, 2017
@kevin-verschaeve
Copy link
Contributor Author

Fine for me too.

What about https://github.com/symfony/symfony/pull/24590/files#diff-ace3ed7f9c8dd7ded3aa562e3c25fa92R36
translation loader should read .yml and .yaml files for translations. It is not the case here

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

Successfully merging this pull request may close these issues.

None yet

6 participants