[Translation] Create an TranslationReaderInterface and move TranslationLoader to TranslationComponent #23667

Nyholm · 2017-07-25T18:21:45Z

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

Nyholm · 2017-07-30T12:35:57Z

I want to ask you about naming.

We have a TranslationWriter which uses DumperInterface.
We also have a TranslationLoader which uses LoaderInterface.

Should this interface be called TranslationReaderInterface instead? Then we got a reader/writer.

javiereguiluz · 2017-08-01T10:28:51Z

👍 Reader/Writer naming looks natural. It's also used by the best programming languages:

Nyholm · 2017-08-01T10:52:25Z

I've renamed the interface now. I'm happy if I could get a review to this PR.

chalasr · 2017-08-01T12:24:28Z

src/Symfony/Component/Translation/Reader/TranslationReaderInterface.php

+     * Reads translation messages from a directory to the catalogue.
+     *
+     * @param string           $directory the directory to look into
+     * @param MessageCatalogue $catalogue the catalogue


descriptions should be ucfirst, both could be removed if you ask me (they echo method description)

chalasr · 2017-08-01T13:05:57Z

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

@@ -22,6 +22,8 @@ CHANGELOG
   name as value, using it makes the command lazy
 * Added `cache:pool:prune` command to allow manual stale cache item pruning of supported PSR-6 and PSR-16 cache pool
   implementations
+ * Deprecated `Symfony\Bundle\FrameworkBundle\Translation\TranslationLoader::loadTranslations` 
+   function, use `Symfony\Bundle\FrameworkBundle\Translation\TranslationLoader::read` instead. 


#23666 should be merged first so this is not needed (given the whole class is deprecated, you just introduce a new class with interface)

chalasr · 2017-08-01T13:34:14Z

src/Symfony/Bundle/FrameworkBundle/Translation/TranslationLoader.php

+    public function loadMessages($directory, MessageCatalogue $catalogue)
+    {
+        $this->read($directory, $catalogue);
+    }


same as https://github.com/symfony/symfony/pull/23667/files#r130601109, no need for deprecating the method if class deprecated first

Nyholm · 2017-08-01T13:52:09Z

Thank you @chalasr. I made this PR simpler by just adding the interface. When/If merged, I will update #23666

chalasr

👍

Nyholm · 2017-08-02T14:18:20Z

Is it anything else I can do to get this PR merged?

fabpot · 2017-08-02T17:02:09Z

I think it would be nice to have the implementation of TranslationReaderInterface in this PR. Merging a PR with an interface that is not even used internally looks weird to me.

Nyholm · 2017-08-02T17:10:20Z

I made 4 different PRs to keep things separate. I think I made a headache for myself. I will add an implementation here and close #23666

fabpot · 2017-08-02T18:12:43Z

@Nyholm Can you rebase to get rid of the merge commit?

fabpot · 2017-08-02T18:13:59Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml

@@ -122,6 +122,7 @@
        </service>

        <service id="translation.loader" class="Symfony\Bundle\FrameworkBundle\Translation\TranslationLoader" public="true" />


Can we add a deprecation for this service?

chalasr · 2017-08-02T18:02:37Z

UPGRADE-3.4.md

@@ -63,6 +63,12 @@ FrameworkBundle
   class has been deprecated and will be removed in 4.0. Use the
   `Symfony\Component\Translation\DependencyInjection\TranslatorPass` class instead.

+ * The `Symfony\Bundle\FrameworkBundle\Translation\TranslationLoaders`
+   class has been deprecated and will be removed in 4.0. Use the


typo TranslationLoaders

chalasr · 2017-08-02T18:03:23Z

UPGRADE-4.0.md

@@ -366,7 +366,13 @@ FrameworkBundle
 * The `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslatorPass`
   class has been removed. Use the
   `Symfony\Component\Translation\DependencyInjection\TranslatorPass` class instead.
+
+ * The `Symfony\Bundle\FrameworkBundle\Translation\TranslationLoaders`


same, extra s

chalasr · 2017-08-02T18:15:37Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml

@@ -122,6 +122,7 @@
        </service>

        <service id="translation.loader" class="Symfony\Bundle\FrameworkBundle\Translation\TranslationLoader" public="true" />
+        <service id="translation.reader" class="Symfony\Component\Translation\Reader\TranslationReader" public="true" />


the new service should be private, but we have to merge #23624 first or make this one private once #23624 merged because the debug:translation command gets it from the container. Let's keep it as is for now

chalasr · 2017-08-02T18:17:08Z

src/Symfony/Component/Translation/Tests/DependencyInjection/TranslationPassTest.php


-        $expected = array('translation.loader' => new ServiceClosureArgument(new Reference('translation.loader')));
+        $expected = array('translation.reader' => new ServiceClosureArgument(new Reference('translation.reader')));


The deprecated service must still be wired by this pass. I suggest to make translation.loader a public alias of translation.reader, marking translation.loader as deprecated

hmm, we can't make it an alias as it has a different API. It must still be wired explicitly, and we'll remove the code when removing the service.

chalasr · 2017-08-02T18:18:57Z

src/Symfony/Bundle/FrameworkBundle/Translation/TranslationLoader.php

 use Symfony\Component\Translation\MessageCatalogue;
-use Symfony\Component\Translation\Loader\LoaderInterface;
+
+@trigger_error(sprintf('The class "%s" has been deprecated. Use "%s" instead. ', TranslationLoader::class, TranslationReader::class), E_USER_DEPRECATED);


...is deprecated since version 3.4 and will be removed in 4.0. Use ...

chalasr · 2017-08-02T18:19:11Z

src/Symfony/Component/Translation/CHANGELOG.md

@@ -7,6 +7,9 @@ CHANGELOG
 * Added `TranslationDumperPass`
 * Added `TranslationExtractorPass`
 * Added `TranslatorPass`
+ * Added `TranslationReader`
+ * Added `TranslationReaderInterface`
+


extra blank line

Nyholm · 2017-08-02T18:56:30Z

src/Symfony/Component/Translation/Tests/DependencyInjection/TranslationPassTest.php

@@ -25,26 +25,33 @@ public function testValidCollector()
        $loader = (new Definition())


@chalasr Could you give this test an extra review?

I tried to make it clear that translation.loader is a tag.

@Nyholm please see #23667 (comment).
Once BC re-added, you have to keep the legacy test (which uses translation.loader service) in addition to the new one (translation.reader).

(marking the legacy test as @legacy with @expectedDeprecation for the default $readerServiceId value that will change in 4.0)

chalasr · 2017-08-02T19:15:29Z

src/Symfony/Component/Translation/DependencyInjection/TranslatorPass.php

    private $loaderTag;

-    public function __construct($translatorServiceId = 'translator.default', $loaderServiceId = 'translation.loader', $loaderTag = 'translation.loader')
+    public function __construct($translatorServiceId = 'translator.default', $readerServiceId = 'translation.reader', $loaderTag = 'translation.loader')


Sorry, I haven't commented on the right place and my comment was not clear enough.
What I mean is that anyone relying on the deprecated translation.loader service must still be able to use it the same as before (for BC). Thus, the compiler pass must still wire it the same as before.

I suggest to:

Keep translation.loader as $readerServiceId default value here (this pass is part of the component which can be used in the wild, changing the default value here is a BC break)

In the constructor, if 'translation.loader' === $readerServiceId then trigger a deprecation saying that the default value will be translation.reader in 4.0 (needs to be documented in UPGRADE-* as well).

Duplicate the process() logic for wiring the new translation.reader service (hardcoding the id, reusing all the things that are injected in $readerServiceId)

Like this we can just change the default value of $readerServiceId to translation.reader in 4.0, removing the duplicated code.

Thank you for clarifying this for me.

Nyholm · 2017-08-03T08:15:54Z

src/Symfony/Component/Translation/DependencyInjection/TranslatorPass.php

    {
+        if ('translation.loader' === $readerServiceId) {
+            @trigger_error('The default value for $readerServiceId will change in 4.0 to "translation.reader".', E_USER_DEPRECATED);


How should I handle all the tests that are failing because of this deprecation notice?

Indeed, we need to mute the deprecation for internal uses by changing this line https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php#L89 to:

- $this->addCompilerPassIfExists($container, TranslatorPass::class); + if (class_exists(TranslatorPass::class)) { + $container->addCompilerPass(new TranslatorPass('translator.default', 'translation.loader', 'translation.loader', false); + }

and change this check:

- if ('translation.loader' === $readerServiceId) { + if ((4 > func_num_args() || false !== func_get_arg(3)) && 'translation.loader' === $readerServiceId) {

That should fix tests (deps=high can remain red because the code is not there on master).

hehe. That is sneaky. Thank you.

chalasr · 2017-08-03T09:27:42Z

@Nyholm this line needs to be updated https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php#L980 for instantiating the TranslatorPass with the same args as FrameworkBundle does. Once fixed we should be OK regarding BC layers :)

Nyholm · 2017-08-03T09:36:33Z

Thank you. I've updated the PR

chalasr · 2017-08-03T11:52:01Z

src/Symfony/Component/Translation/Tests/DependencyInjection/TranslationPassTest.php

+        $container->setDefinition('translation.reader', $reader);
+        $container->setDefinition('translation.xliff_loader', $loader);
+
+        $pass = new TranslatorPass();


I think we would better not mark this test as legacy and instantiate with args of the 4.0 signature ('translator.default', 'translation.reader', 'translation.loader').

I was consider that as well. But if we do we do not have any test coverage on the duplicated code in the process function.

right! Could we test the duplicated code in the legacy test below? It would ensure that both BC and FC layers are ok and would just be removed along with the layers in 4.0.

JhonnyL · 2017-08-03T14:48:17Z

src/Symfony/Component/Translation/Reader/TranslationReader.php

+     *
+     * @param string           $directory the directory to look into
+     * @param MessageCatalogue $catalogue the catalogue
+     */


This is already documented in interface. Should be {@inheritDoc}

chalasr · 2017-08-03T15:35:52Z

UPGRADE-3.4.md

+   class has been deprecated and will be removed in 4.0. Use the
+   `Symfony\Component\Translation\Reader\TranslationReader` class instead.
+
+ * Deprecated `translation.loader` service, use `translation.reader` instead


* The `translation.loader` service has been deprecated and will be removed in 4.0. Use the `translation.reader` service instead.

(for consistency, same applies to UPGRADE-4.0)

missing line for the deprecated $readerServiceId also

chalasr · 2017-08-03T15:44:23Z

UPGRADE-4.0.md

@@ -510,6 +516,8 @@ Translation
 -----------

 * Removed the backup feature from the file dumper classes.
+
+ * The default value for the second parameter of `TranslatiorPass::__construct` (`$readerServiceID`) was changed to "translation.reader".


The default value of the `$readerServiceId` argument of `TranslatorPass::__construct()` has been changed to `"translation.reader"`.

chalasr · 2017-08-03T15:46:27Z

src/Symfony/Bundle/FrameworkBundle/Translation/TranslationLoader.php

- * TranslationLoader loads translation messages from translation files.
- *
- * @author Michel Salib <michelsalib@hotmail.com>
+ * @deprecated Since 3.4. Use Symfony\Component\Translation\Reader\TranslationReader instead


@deprecated since version 3.4 and will be removed in 4.0. [...]

chalasr · 2017-08-03T15:50:16Z

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

@@ -999,7 +999,7 @@ protected function createContainerFromFile($file, $data = array(), $resetCompile
            $container->getCompilerPassConfig()->setOptimizationPasses(array());
            $container->getCompilerPassConfig()->setRemovingPasses(array());
        }
-        $container->getCompilerPassConfig()->setBeforeRemovingPasses(array(new AddAnnotationsCachedReaderPass(), new AddConstraintValidatorsPass(), new TranslatorPass()));
+        $container->getCompilerPassConfig()->setBeforeRemovingPasses(array(new AddAnnotationsCachedReaderPass(), new AddConstraintValidatorsPass(), new TranslatorPass('translator.default', 'translation.loader', 'translation.loader', false)));


hmm we should make it uses the new behavior new TranslatorPass('translator.default', 'translation.reader'), my bad sorry.

chalasr · 2017-08-03T16:26:56Z

src/Symfony/Component/Translation/DependencyInjection/TranslatorPass.php

    {
+        if ((4 > func_num_args() || false !== func_get_arg(3)) && 'translation.loader' === $readerServiceId) {
+            @trigger_error('The default value for $readerServiceId will change in 4.0 to "translation.reader".', E_USER_DEPRECATED);
+        }


@nicolas-grekas could you please have a look at this layer and confirm it is ok?

Nyholm · 2017-08-03T16:39:13Z

Thank you for the review. Ive updated accordingly.

chalasr

👍

Nyholm · 2017-08-06T19:15:17Z

PR is rebased and test are green.

chalasr

some rebase issues

chalasr · 2017-08-06T19:27:42Z

src/Symfony/Bundle/FrameworkBundle/Command/TranslationDebugCommand.php

            $this->extractor = $this->getContainer()->get('translation.extractor');
        }

        $io = new SymfonyStyle($input, $output);

        $locale = $input->getArgument('locale');
        $domain = $input->getOption('domain');
+        /** @var TranslationReaderInterface $reader */
+        $reader = $this->getContainer()->get('translation.reader');


2 lines should be removed (only $this->reader is used)

chalasr · 2017-08-06T19:28:58Z

src/Symfony/Bundle/FrameworkBundle/Command/TranslationDebugCommand.php

@@ -141,14 +141,16 @@ protected function execute(InputInterface $input, OutputInterface $output)
        // BC to be removed in 4.0
        if (null === $this->translator) {
            $this->translator = $this->getContainer()->get('translator');
-            $this->loader = $this->getContainer()->get('translation.loader');
+            $this->reader = $this->getContainer()->get('translation.loader');


translation.reader

Sorry. I must have missed these.

Thank you

chalasr · 2017-08-08T08:34:02Z

ping @symfony/deciders

Nyholm · 2017-08-22T13:00:13Z

Should I rebase this one more time?

nicolas-grekas · 2017-08-22T13:01:40Z

yes please (and squash meanwhile if you want)

fabpot · 2017-08-22T13:07:04Z

Are we sure we want to move away from Loaders and introduce Readers? Loaders are what we are using everywhere else in the framework. Dumpers are also everywhere, except here as we have Writers. Not sure what is best? Be more consistent with other languages or more consistent with Symfony?

Nyholm · 2017-08-22T13:13:43Z

Are we sure we want to move away from Loaders and introduce Readers?

Yes. We do not give up the concept of "Loader" we just rename it for clarity.

Before

We have a TranslationWriter (implements TranslationWriterInterface) which uses DumperInterface.
We also have a TranslationLoader which uses LoaderInterface.

After

We have a TranslationWriter (implements TranslationWriterInterface) which uses DumperInterface.
We also have a TranslationReader (implements TranslationReaderInterface) which uses LoaderInterface.

If we do not rename it to *Reader then we would have a TranslationLoaderInterface and LoaderInterface which is confusing.

I'll rebase and squash shortly.

fabpot · 2017-08-22T13:23:54Z

@Nyholm very clear. Confusion was on my end. Looks good to me.

nicolas-grekas · 2017-08-29T13:51:48Z

@Nyholm can you rebase please?

Nyholm · 2017-08-29T13:55:01Z

Of course.

… to Translation component

nicolas-grekas · 2017-08-29T14:43:19Z

src/Symfony/Bundle/FrameworkBundle/Command/TranslationDebugCommand.php

     */
-    public function __construct($translator = null, TranslationLoader $loader = null, ExtractorInterface $extractor = null)
+    public function __construct($translator = null, TranslationReaderInterface $reader = null, ExtractorInterface $extractor = null)


isn't this a BC break? BC layer needed instead?

hmm the class of the old typehint now implements this interface and is part of FrameworkBundle (which has a conflict for symfony/translation:<3.4 anyway) so this can't break, right?

fabpot · 2017-08-29T20:54:00Z

Thank you @Nyholm.

… 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

Nyholm · 2017-08-30T05:40:50Z

Thank you for the reviews and for merging

carsonbot added Status: Needs Review Feature labels Jul 25, 2017

nicolas-grekas added this to the 3.4 milestone Jul 26, 2017

Nyholm force-pushed the translation-loader-interface branch from e3ad468 to 7c7d315 Compare July 29, 2017 15:51

chalasr reviewed Aug 1, 2017

View reviewed changes

chalasr mentioned this pull request Aug 1, 2017

Move the TranslationLoader to the Translation component. #23666

Closed

Simperfit approved these changes Aug 1, 2017

View reviewed changes

chalasr approved these changes Aug 1, 2017

View reviewed changes

Nyholm changed the title ~~Create an interface for TranslationLoader~~ Create an TranslationReaderInterface and move TranslationLoader to TranslationComponet Aug 2, 2017

Nyholm changed the title ~~Create an TranslationReaderInterface and move TranslationLoader to TranslationComponet~~ [Translation] Create an TranslationReaderInterface and move TranslationLoader to TranslationComponent Aug 2, 2017

fabpot reviewed Aug 2, 2017

View reviewed changes

chalasr requested changes Aug 2, 2017

View reviewed changes

Nyholm force-pushed the translation-loader-interface branch from 8159e80 to 79c4792 Compare August 2, 2017 18:32

Nyholm commented Aug 2, 2017

View reviewed changes

chalasr reviewed Aug 2, 2017

View reviewed changes

Nyholm commented Aug 3, 2017

View reviewed changes

chalasr reviewed Aug 3, 2017

View reviewed changes

JhonnyL reviewed Aug 3, 2017

View reviewed changes

chalasr reviewed Aug 3, 2017

View reviewed changes

chalasr approved these changes Aug 3, 2017

View reviewed changes

Nyholm force-pushed the translation-loader-interface branch from 7732ef8 to b852370 Compare August 6, 2017 19:07

chalasr reviewed Aug 6, 2017

View reviewed changes

Nyholm force-pushed the translation-loader-interface branch from 9c5cef4 to 27da9a6 Compare August 22, 2017 13:22

Nyholm force-pushed the translation-loader-interface branch from 27da9a6 to 95d2096 Compare August 22, 2017 13:25

chalasr mentioned this pull request Aug 28, 2017

[Translator] Remove deprecated feature #24014

Merged

Create an interface for TranslationReader and moved TranslationLoader…

5bc50da

… to Translation component

Nyholm force-pushed the translation-loader-interface branch from 78661af to 5bc50da Compare August 29, 2017 14:40

nicolas-grekas reviewed Aug 29, 2017

View reviewed changes

fabpot approved these changes Aug 29, 2017

View reviewed changes

fabpot merged commit 5bc50da into symfony:3.4 Aug 29, 2017

Nyholm deleted the translation-loader-interface branch August 30, 2017 06:23

javiereguiluz mentioned this pull request Sep 6, 2017

[Translation] Create an TranslationReaderInterface and move TranslationLoader to TranslationComponent symfony/symfony-docs#8363

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

		@@ -122,6 +122,7 @@
		</service>

		<service id="translation.loader" class="Symfony\Bundle\FrameworkBundle\Translation\TranslationLoader" public="true" />


		$expected = array('translation.loader' => new ServiceClosureArgument(new Reference('translation.loader')));
		$expected = array('translation.reader' => new ServiceClosureArgument(new Reference('translation.reader')));

		@@ -25,26 +25,33 @@ public function testValidCollector()
		$loader = (new Definition())

[Translation] Create an TranslationReaderInterface and move TranslationLoader to TranslationComponent #23667

[Translation] Create an TranslationReaderInterface and move TranslationLoader to TranslationComponent #23667

Conversation

Nyholm commented Jul 25, 2017 • edited Loading

Nyholm commented Jul 30, 2017

javiereguiluz commented Aug 1, 2017

Nyholm commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Aug 1, 2017 • edited Loading

chalasr left a comment

Choose a reason for hiding this comment

Nyholm commented Aug 2, 2017

fabpot commented Aug 2, 2017

Nyholm commented Aug 2, 2017

fabpot commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Aug 3, 2017 • edited Loading

Nyholm commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Aug 3, 2017

chalasr left a comment

Choose a reason for hiding this comment

Nyholm commented Aug 6, 2017

chalasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Aug 8, 2017

Nyholm commented Aug 22, 2017

nicolas-grekas commented Aug 22, 2017

fabpot commented Aug 22, 2017

Nyholm commented Aug 22, 2017 • edited Loading

Before

After

fabpot commented Aug 22, 2017

nicolas-grekas commented Aug 29, 2017

Nyholm commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Aug 29, 2017

Nyholm commented Aug 30, 2017

Nyholm commented Jul 25, 2017 •

edited

Loading

Nyholm commented Aug 1, 2017 •

edited

Loading

chalasr Aug 2, 2017 •

edited

Loading

chalasr commented Aug 3, 2017 •

edited

Loading

Nyholm commented Aug 22, 2017 •

edited

Loading