Added Symfony 4 compatibility and refactored code #345

junaidbinfarooq · 2019-11-13T06:21:33Z

Replaced Symfony\Bundle\FrameworkBundle\Controller\Controller with Symfony\Bundle\FrameworkBundle\Controller\AbstractController
Added proper return type and type hints
Updated php doc-blocks

Replaced Symfony\Bundle\FrameworkBundle\Controller\Controller with Symfony\Bundle\FrameworkBundle\Controller\AbstractController Added proper return type and type hints Updated php doc-blocks

odolbeau

Thanks for your first contribution. :) It looks great. 👍
Could you please update CS?

XWB · 2019-11-13T11:50:34Z

You will need to replace the $this->get() calls as well, because AbstractController uses a smaller container that does not contain all services.

welcoMattic

Please run cs-fixer to clean PHPDoc blocs ;)
Thanks for your contribution

welcoMattic · 2019-11-13T14:17:54Z

Controller/EditInPlaceController.php

 {
    /**
+     * @param Request $request
     * @param string $configName
     * @param string $locale


cs-fixer should remove those PHPDoc lines since they are redundant with type hinting.

#342 also has the commits that fix cs, but these did not make it to master yet

Ok, let's merge #342 and this one. 👍

+ Removed $this-get calls in the controller and injected appropriate dependencies via constructor + cs-fixes + Added proper type-hints and return types + Added ext-json in the composer.json file

junaidbinfarooq · 2019-11-19T17:48:46Z

Thanks for your first contribution. :) It looks great.
Could you please update CS?

@odolbeau Updated the PR with CS-fixes and more changes. Could you please have a look.

XWB · 2019-11-19T17:53:56Z

@junaidbinfarooq Much better indeed. Still, those changes will only work if you have autowire enabled. So you'll have to create Resources/config/controller.yml and add each controller as a service.

junaidbinfarooq · 2019-11-20T10:22:45Z

@junaidbinfarooq Much better indeed. Still, those changes will only work if you have autowire enabled. So you'll have to create Resources/config/controller.yml and add each controller as a service.

Oh! yeah. Done now. Please have a look.

XWB · 2019-11-20T10:33:56Z

Resources/config/services.yml

@@ -1,3 +1,6 @@
+imports:
+    - { resource: controller.yml }


Being a bundle, it would be better to move the import to TranslationExtension. See https://github.com/php-translation/symfony-bundle/blob/master/DependencyInjection/TranslationExtension.php#L42

Did see this before but still thought of importing the services as we do in the applications. Anyways, this is done now.

XWB · 2019-11-20T10:34:43Z

Resources/config/controller.yml

@@ -0,0 +1,9 @@
+services:
+    _defaults:


The _defaults section needs to be removed. Autowire must be enabled in your own application and not in a bundle.

XWB · 2019-11-20T10:35:25Z

composer.json

@@ -22,7 +22,8 @@
        "php-translation/symfony-storage": "^1.0",
        "php-translation/extractor": "^1.6",
        "nyholm/nsa": "^1.1",
-        "twig/twig": "^1.42 || ^2.11"
+        "twig/twig": "^1.42 || ^2.11",
+        "ext-json": "*"


Is this change required?

Added this after encountering few usages of json_encode/decode functions. PhpStorm does show a warning for this.

XWB · 2019-11-20T10:35:52Z

@junaidbinfarooq Almost done :)

XWB · 2019-11-20T10:37:18Z

Resources/config/controller.yml

+        autoconfigure: true
+        public: true
+
+    Translation\Bundle\Controller\:


You will need to inject all the services into the constructor, so the controllers can work without autowire.

Have indeed used constructor injection.

…Extension + Renamed controller.yml to controllers.yml + Removed a few non-existent class usages

welcoMattic · 2019-11-27T09:49:32Z

@junaidbinfarooq Hi, as #342 is now merged in master, could you fix conflicts due to the merge (#342 (comment)).
Thank you 👍

+ Minor code refactor

junaidbinfarooq · 2019-11-28T06:01:10Z

@junaidbinfarooq Hi, as #342 is now merged in master, could you fix conflicts due to the merge (#342 (comment)).
Thank you

Done. Could you please have a look.

XWB · 2019-11-28T13:43:34Z

Looks good in general. Just some tests need to be fixed.

junaidbinfarooq · 2019-12-31T08:31:18Z

@odolbeau Couldn't you have created a fork of my branch and update the tests instead of creating another PR and closing this one?

odolbeau · 2019-12-31T12:34:11Z

I tried but I finally choose to create a new branch for several reasons.
Your branch mix 2 things: the Symfony 5 migration & the Strict type hinting. This branch was quite outdated compared to master when I started to work. Last but not least, you weren't active since a few weeks.
I hope you'll understand why I did that.
Thanks for your work anyway! :)

Added Symfony 4 compatibility and refactored code

9db41a0

Replaced Symfony\Bundle\FrameworkBundle\Controller\Controller with Symfony\Bundle\FrameworkBundle\Controller\AbstractController Added proper return type and type hints Updated php doc-blocks

odolbeau approved these changes Nov 13, 2019

View reviewed changes

XWB added this to the Version 1.0 milestone Nov 13, 2019

XWB self-requested a review November 13, 2019 11:53

welcoMattic requested changes Nov 13, 2019

View reviewed changes

+ Replaced Controller with AbstractController in the controllers

0b6c5b5

+ Removed $this-get calls in the controller and injected appropriate dependencies via constructor + cs-fixes + Added proper type-hints and return types + Added ext-json in the composer.json file

junaidbinfarooq added 2 commits November 20, 2019 15:49

+ Registered controllers as services

2223f92

+ Removed accidental commit

fa9c8d7

junaidbinfarooq requested a review from welcoMattic November 20, 2019 10:23

XWB requested changes Nov 20, 2019

View reviewed changes

XWB reviewed Nov 20, 2019

View reviewed changes

+ Now importing the new service file controllers.yml in Translation…

f6cebf5

…Extension + Renamed controller.yml to controllers.yml + Removed a few non-existent class usages

junaidbinfarooq requested a review from XWB November 20, 2019 19:27

+ CS-fix

5a1dfcf

junaidbinfarooq requested a review from rvanlaak November 22, 2019 09:25

odolbeau mentioned this pull request Nov 27, 2019

Add I/O type hinting, use null coalescing operator #342

Merged

welcoMattic approved these changes Nov 27, 2019

View reviewed changes

junaidbinfarooq and others added 7 commits November 28, 2019 10:25

Merge branch 'master' into patch-1

b128fc0

Add I/O type hinting, use null coalescing operator

a48fb01

documentation fixes for phpstan lvl 2

b2597de

Throw Exception if no configuration found, update I/O type hinting

8395971

Fix CS

89d1df3

+ CS-fix

74234e3

+ CS-fix

e61d799

+ Minor code refactor

welcoMattic mentioned this pull request Nov 29, 2019

allow symfony 5 #346

Merged

odolbeau mentioned this pull request Dec 7, 2019

SF 5.x compatibility #356

Merged

3 tasks

odolbeau closed this Dec 26, 2019

junaidbinfarooq deleted the patch-1 branch December 28, 2019 18:24

Added Symfony 4 compatibility and refactored code #345

Added Symfony 4 compatibility and refactored code #345

Uh oh!

Conversation

junaidbinfarooq commented Nov 13, 2019

Uh oh!

odolbeau left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XWB commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

welcoMattic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvanlaak Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junaidbinfarooq commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XWB commented Nov 19, 2019

Uh oh!

junaidbinfarooq commented Nov 20, 2019

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!

XWB commented Nov 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

welcoMattic commented Nov 27, 2019

Uh oh!

junaidbinfarooq commented Nov 28, 2019

Uh oh!

XWB commented Nov 28, 2019

Uh oh!

junaidbinfarooq commented Dec 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odolbeau commented Dec 31, 2019

Uh oh!

Uh oh!

odolbeau left a comment •

edited

Loading

XWB commented Nov 13, 2019 •

edited

Loading

rvanlaak Nov 13, 2019 •

edited

Loading

junaidbinfarooq commented Nov 19, 2019 •

edited

Loading

junaidbinfarooq commented Dec 31, 2019 •

edited

Loading