Add Symfony 4 support #145

bocharsky-bw · 2017-12-12T22:00:36Z

No description provided.

Nyholm · 2017-12-13T07:50:36Z

Thank you. I will make sure the tests are green later today.

bocharsky-bw · 2017-12-13T09:02:07Z

I was able to fix some of them, but I still have some weird fails.

Also, StyleCI shows some fails not related to this PR. Should I fix them here?

bocharsky-bw · 2017-12-16T10:17:37Z

This PR is blocked by php-translation/symfony-storage#19

bocharsky-bw · 2017-12-16T10:32:04Z

Finally, I was able to fix all the tests, but they pass only with php-translation/symfony-storage#19 patch

…to patch-1

Nyholm

Thank you.

We need a legacy layer for the TranslationReader

Nyholm · 2017-12-20T11:22:00Z

Catalogue/CatalogueFetcher.php

     */
-    public function __construct(TranslationLoader $loader)
+    public function __construct(TranslationReader $reader)


This class does not exists in sf3.4

Yeah, but I'm not sure what's the best way to inject wether translation.reader or translation.loader depends on Symfony version. Injecting the whole container probably is not an option because those services are private. Maybe Compiler Pass can help with it?

Compiler pass would be preferable as that moves the calculation from runtime to compile time. 👍

Unfortunately TranslationLoader does not have the TranslationReaderInterface yet, so typehinting won't be possible right?

@bocharsky-bw I've made a PR to your fork. Please have a look.

Nyholm · 2017-12-20T11:22:32Z

Resources/config/extractors.yml

@@ -1,10 +1,12 @@
 services:
  php_translation.extractor.php:
+    public: true


Do these really have to be public?

bocharsky-bw · 2017-12-21T09:27:54Z

One more blocker for this PR: php-translation/symfony-storage#22

symfony-storage tests had not revealed it, but symfony-bundle tests did

…troller

Translation Loader or Reader and Definitions (BC SF3.3+)

bocharsky-bw · 2017-12-25T19:08:08Z

@Nyholm finally this PR is done and ready for your review

Nyholm

Thank you

Nyholm · 2017-12-27T11:53:01Z

Catalogue/CatalogueFetcher.php

 use Translation\Bundle\Model\Configuration;
+use Translation\SymfonyStorage\LegacyTranslationLoader;
+use Translation\SymfonyStorage\TranslationLoader;


This interface does not exist anymore

Nyholm · 2017-12-27T11:53:34Z

Catalogue/CatalogueFetcher.php

     */
-    public function __construct(TranslationLoader $loader)
+    public function __construct($loader)


This should be renamed to "$reader"

Nyholm · 2017-12-27T11:54:06Z

Catalogue/CatalogueFetcher.php

    {
+        // Create a legacy loader which is a wrapper for TranslationReader
+        if ($loader instanceof TranslationReader) {


if (!$loader instanceof TranslationReaderInterface)

Nyholm · 2017-12-27T11:54:42Z

Catalogue/CatalogueFetcher.php

+        if ($loader instanceof TranslationReader) {
+            $loader = new LegacyTranslationLoader($loader);
+        }
+        if (!$loader instanceof SymfonyTranslationLoader && !$loader instanceof TranslationLoader) {


This is not needed if you change the logic of the if above

Nyholm · 2017-12-27T11:55:19Z

Catalogue/CatalogueWriter.php

@@ -53,7 +53,7 @@ public function __construct(
    public function writeCatalogues(Configuration $config, array $catalogues)
    {
        foreach ($catalogues as $catalogue) {
-            $this->writer->writeTranslations(
+            $this->writeTranslations(


We should use $this->writer->write()

Nyholm · 2017-12-27T11:56:12Z

DependencyInjection/CompilerPass/LoaderOrReaderPass.php

+
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+


Could you write a class comment that explains the purpose of this CompilerPass?

Nyholm · 2017-12-27T11:57:47Z

Resources/config/edit_in_place.yml

@@ -14,6 +14,10 @@ services:
    class: Translation\Bundle\EditInPlace\Activator
    arguments: ['@session']

+  test.php_translation.edit_in_place.activator:


Could we put these in a separate file?

Nyholm · 2017-12-27T11:58:00Z

Resources/config/services.yml

    class: Translation\Bundle\Catalogue\CatalogueFetcher
-    arguments: ["@translation.loader"]
+    arguments: ["@translation.loader_or_reader"]


Yeah, thanks to @dkozickis for this idea and his help

Nyholm · 2017-12-27T11:58:22Z

Resources/config/services.yml

@@ -13,26 +15,37 @@ services:
    arguments: ["@php_translation.catalogue_fetcher", "@php_translation.catalogue_writer", ~]

  php_translation.catalogue_manager:
+    public: true


Do the services need to be public?

Yes, we call it from the container in WebUIController::showAction()

We can inject it into controller instead, no?

Yeah, we should But that is out of scope for this PR

IIRC, it won't work in 2.7, will it?

Nyholm · 2017-12-27T11:58:49Z

composer.json


        "php-translation/common": "^0.2.1",
-        "php-translation/symfony-storage": "^0.3.2",
+        "php-translation/symfony-storage": "^0.3.4",


Should be 0.4.0

bocharsky-bw · 2017-12-28T13:43:49Z

@Nyholm All the requested changes are done!

Nyholm

Great. Just some minors

Nyholm · 2017-12-28T13:53:25Z

Catalogue/CatalogueFetcher.php

 use Translation\Bundle\Model\Configuration;
+use Translation\SymfonyStorage\LegacyTranslationReader;
+use Translation\SymfonyStorage\TranslationLoader;


This interface does not exits anymore.

What do you mean? Because I see it in master: https://github.com/php-translation/symfony-storage/blob/master/src/TranslationLoader.php and it is not even deprecated.

Nyholm · 2017-12-28T13:55:28Z

Command/ExtractCommand.php

@@ -39,7 +39,7 @@ protected function configure()
    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $container = $this->getContainer();
-        $importer = $container->get('php_translation.importer');
+        $importer = $container->get('test.php_translation.importer');


Hm... This is not perfect. Lets make php_translation.importer public for now.

Just fix this and I'm happy to merge

Good catch! Sorry about this

Nyholm · 2017-12-28T14:07:36Z

You are correct. I meant o remove it.. Leave it like this for now. Med vänliga hälsningar Tobias Nyholm

…

On 28 Dec 2017, at 15:05, Victor Bocharsky ***@***.*** ***@***.***>> wrote: @bocharsky-bw commented on this pull request. In Catalogue/CatalogueFetcher.php <#145 (comment)>: > use Translation\Bundle\Model\Configuration; +use Translation\SymfonyStorage\LegacyTranslationReader; +use Translation\SymfonyStorage\TranslationLoader; What do you mean? Because I see it in master: https://github.com/php-translation/symfony-storage/blob/master/src/TranslationLoader.php <https://github.com/php-translation/symfony-storage/blob/master/src/TranslationLoader.php> and it is not even deprecated. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#145 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABN1RpBWOTtf_HaMbYwRCbkPCgsVXw8kks5tE6AigaJpZM4Q_srX>. <https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png> <https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png> <https://github.com/php-translation/symfony-bundle> <#145 (comment)>

Nyholm

Awesome. Thanks!

Add Symfony 4 support

ff02e83

Nyholm self-requested a review December 12, 2017 22:00

bocharsky-bw added 2 commits December 13, 2017 10:17

Use ChildDefinition instead of DefinitionDecorator

24882e4

Make some services public and fix deprecated class calls

a16f916

Fix more tests

6fb3429

bocharsky-bw mentioned this pull request Dec 16, 2017

Add Symfony 4 support php-translation/symfony-storage#19

Merged

Fix remaining tests: Make more services public

1c4e527

bocharsky-bw and others added 2 commits December 17, 2017 12:29

Merge branch 'master' of github.com:php-translation/symfony-bundle in…

75dc718

…to patch-1

Merge branch 'master' into patch-1

72a3306

Nyholm reviewed Dec 20, 2017

View reviewed changes

bocharsky-bw and others added 8 commits December 21, 2017 12:35

Call write() when exists instead of deprecated writeTranslations()

7391edc

Make php_translation.cache_clearer service public - we call it in con…

e114a06

…troller

Revert php-translation/symfony-storage version in composer.json

4844b40

Revert making php_translation.extractor.php public - use public alias

5887a83

Revert making php_translation.extractor.twig public - use public alias

23762ba

Revert making php_translation.extractor public - use public alias

cfa14a1

Revert making one more service public in favor of public alias

4f23256

* Translation Loader or Reader (BC SF3.4)

2931222

dkozickis mentioned this pull request Dec 24, 2017

Translation Loader or Reader and Definitions (BC SF3.3+) bocharsky-bw/symfony-bundle#1

Merged

dkozickis and others added 6 commits December 25, 2017 17:18

* StyleCI

a54d3d6

* Legacy wrapper for ChildDefiniton/DefinitionDecorator

d54ee4f

* StyleCI fix

2d74ebc

* One more fix for translation loader or reader

06fc223

Merge pull request #1 from dkozickis/patch-1

9594986

Translation Loader or Reader and Definitions (BC SF3.3+)

Tweaked service name to translation.loader_or_reader

f40e62e

bocharsky-bw added 3 commits December 25, 2017 19:56

Bump php-translation/symfony-storage version to ^0.3.4

f14ee83

Fix styles: Add missing header for new PHP classes

271672f

Create LegacyTranslationLoader on compilation to avoid extra checks

4469cfe

bocharsky-bw mentioned this pull request Dec 27, 2017

Create legacy class instead of method to improve performance php-translation/symfony-storage#23

Merged

Nyholm requested changes Dec 27, 2017

View reviewed changes

bocharsky-bw added 4 commits December 28, 2017 14:40

Bump symfony-storage version to v0.4.0

97671a8

Move test.* services to a separate file and include it for tests

d922523

Add a description for LoaderOrReaderPass class

5b0458e

Use new LegacyTranslationReader and LegacyTranslationWriter classes

66bd35e

Nyholm requested changes Dec 28, 2017

View reviewed changes

Make php_translation.importer public and remove its test alias

b0dddbb

Nyholm approved these changes Dec 28, 2017

View reviewed changes

Nyholm merged commit e6b000b into php-translation:master Dec 28, 2017

bocharsky-bw deleted the patch-1 branch December 28, 2017 17:27


		use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
		use Symfony\Component\DependencyInjection\ContainerBuilder;

Add Symfony 4 support #145

Add Symfony 4 support #145

Uh oh!

Conversation

bocharsky-bw commented Dec 12, 2017

Uh oh!

Nyholm commented Dec 13, 2017

Uh oh!

bocharsky-bw commented Dec 13, 2017

Uh oh!

bocharsky-bw commented Dec 16, 2017

Uh oh!

bocharsky-bw commented Dec 16, 2017

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bocharsky-bw commented Dec 21, 2017

Uh oh!

bocharsky-bw commented Dec 25, 2017

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bocharsky-bw commented Dec 28, 2017

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Dec 28, 2017 via email