Move the TranslationLoader to the Translation component. #23666
Conversation
|
Not sure why the "deps high" test is failing. Should I split this into two PRs? |
|
it fails because it runs on master for which the class does not exist yet (it's ok). |
| @@ -37,6 +37,9 @@ FrameworkBundle | |||
| * The `--no-prefix` option of the `translation:update` command is deprecated and | |||
| will be removed in 4.0. Use the `--prefix` option with an empty string as value | |||
| instead (e.g. `--prefix=""`) | |||
|
|
|||
| * The class `Symfony\Bundle\FrameworkBundle\Translation\TranslationLoader` was | |||
| moved to the Translation component (`Symfony\Component\Translation\Loader\TranslationLoader`) | |||
There was a problem hiding this comment.
You need to also add a note in the CHANGELOG for 4.0.
|
Not sure if it's really needed, but you can easily remove the Finder dependency because the code using it is trivial. It's a matter of making a single |
|
Thank you for the reviews. I've updated accordingly. @javiereguiluz I prefer to leave it as it is. The Finder dependency is not large and I may have more PRs later that depend on Finder. |
| */ | ||
| class TranslationLoader | ||
| class TranslationLoader extends \Symfony\Component\Translation\Loader\TranslationLoader |
There was a problem hiding this comment.
You should add a deprecation notice when this class is used.
There was a problem hiding this comment.
And internal usages of this class should be changed to use the new class instead (which could lead to some composer dep changes in frameworkbundle probably)
| $catalogue->addCatalogue($loader->load($file->getPathname(), $catalogue->getLocale(), $domain)); | ||
| } | ||
| } | ||
| @trigger_error(sprintf('The class "%s" has been deprecated. Use "%s" instead. ', self::class, \Symfony\Component\Translation\Loader\TranslationLoader::class), E_USER_DEPRECATED); |
There was a problem hiding this comment.
the call to trigger_error should be just after the namespace (see e.g. https://github.com/symfony/symfony/blob/3.4/src/Symfony/Bundle/TwigBundle/Command/DebugCommand.php#L14)
There was a problem hiding this comment.
I did not know. Thank you.
|
The changelog files for the FrameworkBundle and the Translation component should be updated for 3.4. |
6954c9c to
8210985
Compare
|
Thank you for the feedback. I've added change logs. |
| // load any existing translation files | ||
| $finder = new Finder(); | ||
| $extension = $catalogue->getLocale().'.'.$format; | ||
| $files = $finder->files()->name('*.'.$extension)->in($directory); |
There was a problem hiding this comment.
I would drop the finder requirement for an iterator.
There was a problem hiding this comment.
Already pointed by @javiereguiluz but @Nyholm said it would need the Finder dep anyway for some other changes he wants to make. So, probably good as is.
|
Should probably be renamed to TranslationReader (#23667). |
|
I've moved the changes in this PR to #23667. The PR's were too tightly coupled. |
… move TranslationLoader to TranslationComponent (Nyholm) This PR was merged into the 3.4 branch. Discussion ---------- [Translation] Create an TranslationReaderInterface and move TranslationLoader to TranslationComponent | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Create an interface for TranslationLoader. I put this in the Translation component even though the TranslationLoader implementation is in the FrameworkBundle. (Here is a PR moving the TranslationLoader to the Translation component: #23666) We need this in PHP-translation. We currently have to rewrite things in strange mannar like: https://github.com/php-translation/symfony-storage/blob/0.2.2/src/FileStorage.php#L62-L64 Commits ------- 5bc50da Create an interface for TranslationReader and moved TranslationLoader to Translation component
Move the TranslationLoader away from the FrameworkBundle and to the Translation component where it belongs. Im not sure why it was put here in the first place. My guess is that we wanted to avoid adding Finder as a dependency in the Translation component.
Btw, this is me making this suggestion very politely. I think it makes sense since third party libraries should not be forced to require all FrameworkBundle for this class.