Skip to content

[Translation][Command][FrameworkBundle] Enable translation debugging in directories #13443

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

Merged
merged 1 commit into from
Apr 6, 2015

Conversation

xelaris
Copy link
Contributor

@xelaris xelaris commented Jan 18, 2015

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

This PR follows up #13340 and enables not only to inspect Bundles, but also directories, like the app/ directory.

Additionally it harmonizes the TranslationDebugCommand and TranslationUpdateCommand to expect an optional bundle name or a directory and fall back to kernel root dir if none of them is given.

@stof
Copy link
Member

stof commented Jan 18, 2015

This is now making it impossible to debug the whole translations

@xelaris
Copy link
Contributor Author

xelaris commented Jan 18, 2015

@stof What do you mean with "the whole translations"? Before the change a bundle name was already mandatory.

@xelaris
Copy link
Contributor Author

xelaris commented Mar 29, 2015

ping @stof I'm not sure what to do. Could you explain your last comment, please?

@aitboudad
Copy link
Contributor

@xelaris, stof means if we put the defaults value to app/Resources folder, there is no way to debug all translations available in the app.

@xelaris
Copy link
Contributor Author

xelaris commented Mar 29, 2015

How was it possible before? Since the bundle argument of the debug:translation command was defined as InputArgument::REQUIRED it was not possible to call the command without a bundle name. So I can't see the limitation in making it optional and falling back to app/Resources. Maybe I overlook something?

@aitboudad
Copy link
Contributor

IMO if we want to set a default value for this command I think it should be the whole translations.

@xelaris
Copy link
Contributor Author

xelaris commented Mar 29, 2015

Ok, I got the point. We could keep the bundle argument required, but still enable to provide a path besides a bundle name. But this in turn would be inconsistent with the translation:update command where the argument falls back to app/Resources and I think it is not very intuitive. On the other hand I agree, it would be nice to debug the whole applications translations, and at first glance the most intuitive way to do this would to call the command without a bundle name.
However following the defined best practice, the most common way is to keep all app translations in app/Resources. And hence the main use case would to call the command to debug translations in app/Resources. It would may be nice to make this call the shortest one. IMO Debugging translations of all bundles would be a less common use case. What about an --all option, which could be passed, if someone would debug the whole application?

@aitboudad
Copy link
Contributor

--all sound good to me

@aitboudad
Copy link
Contributor

@xelaris any news ?

@xelaris
Copy link
Contributor Author

xelaris commented Apr 4, 2015

I think an --all option is out of scope of this PR, since it adds a new feature. I can start working on it, but don't know when I get to it. So what about merging this PR (if everything else is fine) and making it possible to debug all translations in a subsequent PR?

@aitboudad
Copy link
Contributor

I'm ok, then can we also move the defaults value(app/Resources folder) feature ?

@aitboudad
Copy link
Contributor

@xelaris TranslationUpdateCommand use defaults to app/Resources folder, forget my previous comment :)

@aitboudad
Copy link
Contributor

👍 ping @fabpot

@aitboudad
Copy link
Contributor

@xelaris can apply the below patch and rebase your branch:

@@ -116,7 +116,9 @@ EOF

         // Extract used messages
         $extractedCatalogue = new MessageCatalogue($locale);
-        $this->getContainer()->get('translation.extractor')->extract($rootPath.'/Resources/views', $extractedCatalogue);
+        if (is_dir($translationsPath)) {
+            $this->getContainer()->get('translation.extractor')->extract($translationsPath, $extractedCatalogue);
+        }

         // Load defined messages
         $currentCatalogue = new MessageCatalogue($locale);

@aitboudad
Copy link
Contributor

once you made the change I'll merge :)

Harmonize TranslationDebugCommand and TranslationUpdateCommand to
expect an optional bundle name or a directory and fall back to kernel
root dir if none of them is given.
@xelaris xelaris force-pushed the debug-translation-27 branch from e9e7e07 to 2662244 Compare April 6, 2015 15:10
@xelaris
Copy link
Contributor Author

xelaris commented Apr 6, 2015

@aitboudad The branch is rebased and I added the is_dir check from the patch, but kept $rootPath.'/Resources/views', since $translationsPath contains $rootPath.'/Resources/translations'.

@aitboudad
Copy link
Contributor

@xelaris ok nice !

@aitboudad aitboudad changed the title [FrameworkBundle] Enable translation debugging in directories [Translation][Command][FrameworkBundle] Enable translation debugging in directories Apr 6, 2015
@aitboudad
Copy link
Contributor

Thank you @xelaris.

@aitboudad aitboudad merged commit 2662244 into symfony:2.7 Apr 6, 2015
aitboudad added a commit that referenced this pull request Apr 6, 2015
…tion debugging in directories (xelaris)

This PR was merged into the 2.7 branch.

Discussion
----------

[Translation][Command][FrameworkBundle] Enable translation debugging in directories

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

This PR follows up #13340 and enables not only to inspect Bundles, but also directories, like the `app/` directory.

Additionally it harmonizes the TranslationDebugCommand and TranslationUpdateCommand to expect an optional bundle name or a directory and fall back to kernel root dir if none of them is given.

Commits
-------

2662244 [FrameworkBundle] Enable translation debugging in directories
@aitboudad
Copy link
Contributor

@xelaris can you open an issue for --all option, if someone wants to work on it?

@xelaris
Copy link
Contributor Author

xelaris commented Apr 6, 2015

Thanks for merging @aitboudad, an issue for the --all option is created.

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.

4 participants