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

ker0x · 2019-10-30T14:49:54Z

As the package require PHP >= 7.1, I added i/o type hinting when possible and replace some isset() call with the null coalescing operator

bocharsky-bw

Wow, a lot of changes, thanks for doing this routine work!

So, this PR contains BC breaks, right? As I understand, if someone overrides some classes from this bundle - they will get fatal errors after upgrade because some method signatures in this bundle were changed. Or am I wrong?

Catalogue/CatalogueManager.php

bocharsky-bw · 2019-11-04T08:50:12Z

Command/DeleteObsoleteCommand.php

    {
        $configName = $input->getArgument('configuration');
        $locales = [];
        if (null !== $inputLocale = $input->getArgument('locale')) {
            $locales = [$inputLocale];
        }

-        $config = $this->configurationManager->getConfiguration($configName);
+        if (null === $config = $this->configurationManager->getConfiguration($configName)) {
+            return;


I'm not sure about this.... Here you changed behaviour, are you really sure that with null config nothing is happen further?

@bocharsky-bw just after, line 93 this method is called which has the $config typed to Configuration (not from this PR). Either it will throw an Exception for the wrong type or for calling method reconfigureBundleDirs() on null.

However, I can add

$output->writeln(\sprintf('No configuration found for %s, $configName));

so users will not be confused.

Ah, I see... Hm, so it seems before it would fail with an exception. Then, I think, call writeln() at least is a good idea here. Or, probably even better to throw an exception? Because without that config the command really do nothing here

Changed in 6e5d463, this will throw an Exception in Command classes but maybe this can be directly deported in the getConfiguration() method ?

Great!

but maybe this can be directly deported in the getConfiguration() method ?

Sounds good at the first sight, but let's do it in a separate PR I think

Command/DownloadCommand.php

Controller/WebUIController.php

Model/CatalogueMessage.php

bocharsky-bw · 2019-11-04T09:57:32Z

Service/StorageManager.php

    {
        $this->storages[$name] = $storage;
    }

    /**
-     * @param string $name
+     * @param string|string[]|null $name


Hm, but we do not have an implementation when $name isstring[], right? Here, and in a few more places

This method is called in EditInPlaceController and WebUIController which required $configName to be a string. But it's also called in method getStorage of StorageTrait and here, $configName is provide with $input->getArgument('configuration') from some Command classes and return types of getArgument() are string|string[]|null

rvanlaak · 2019-11-04T10:59:19Z

Nice job! The change in #334 on raising the phpstan level could help you with what you're trying to achieve with this PR 👍

Nyholm · 2019-11-04T15:58:49Z

Wohoo. Thank you!

rvanlaak · 2019-11-05T09:33:26Z

How does everyone feel about removing all phpdoc that can get determined based on the typehints for methods without a description?

Nyholm · 2019-11-05T09:49:07Z

I think that is a good idea. There is a PHP-cs-fixer rule for that.

I suggest that we address the issues pointed out by @bocharsky-bw and then merge this PR. After that we adjust the CS rules to make fix @rvanlaak suggestion.

This PR could get too big, too hard to review and fix too many things otherwise.

rvanlaak · 2019-11-05T09:58:46Z

I locally tested raising phpstan to level 2, and started fixed the documentation errors phpstan reports. Created PR ker0x#1 to raise PHPStan to lvl2.

Found a bug on the return signature of SfProfilerMessage::setKey, which is changed to string but should be self. Fixed that in 083bff5

Also reported #343 as result of that.

So, yes let's merge this PR once the review is processed 👏

@Nyholm did you possibly change PHP-CS-Fixer config already? The PHP-CS-Fixer job is failing and the proposed changes look like what you suggested above by changing the CS rule.

ker0x · 2019-11-05T17:32:18Z

@rvanlaak we faced the same issue today at my work. The change come from PHP-CS-Fixer itself which as a new default configuration for the @Symfony and @PhpCsFixer rule: no_superfluous_phpdoc_tags

rvanlaak · 2019-11-06T09:08:40Z

Created #344 so this PR can focus on adding type hints 👏

Raise phpstan to level 2

rvanlaak · 2019-11-08T12:27:39Z

So, this PR contains BC breaks, right? As I understand, if someone overrides some classes from this bundle - they will get fatal errors after upgrade because some method signatures in this bundle were changed. Or am I wrong?

@bocharsky-bw great review! Yes, definitely a BC break, but as 1.0 of this bundle has not been released yet I'd be kinda leaning towards being ok with that ;-)

rvanlaak · 2019-11-11T08:38:18Z

Tests/Unit/Model/ConfigurationTest.php

@@ -58,7 +55,7 @@ public static function getDefaultData()
            'output_format' => 'getOutputFormat',
            'blacklist_domains' => ['getBlacklistDomains'],
            'whitelist_domains' => ['getWhitelistDomains'],
-            'xliff_version' => ['getXliffVersion'],
+            'xliff_version' => 'getXliffVersion',


Would this be fixing a bug?

Yes because xliff_version is of type scalarNode not arrayNode, https://github.com/php-translation/symfony-bundle/blob/master/DependencyInjection/Configuration.php#L164

rvanlaak · 2019-11-13T16:03:12Z

Asked @Nyholm for a final review, so he can approve and then merge this PR 👍

rvanlaak · 2019-11-25T08:28:43Z

This PR most likely is going to give conflicts once the smaller PRs would get merged.

@bocharsky-bw @odolbeau shall we merge this?

bocharsky-bw · 2019-11-27T09:27:49Z

@rvanlaak Hm, good question. Usually, I'd say it's fair enough to merge the one that was created earlier and then fix merge conflicts in the 2nd... but this PR is special, it really has a lot of changes. So, probably merge it first is a better idea 👍

odolbeau · 2019-11-27T09:32:03Z

It's definitively OK to merge this PR IMO!
Let's merge it & we'll be able to take care of #345 & #346 before releasing a v1.0 version I think. 🎉

welcoMattic · 2019-11-27T09:36:23Z

I'm ok with @odolbeau and @bocharsky-bw, so I merge this one, and we will handle #345 and #346 before the end of the week if it's possible 🎉
Get ready for v1.0 😉

ker0x force-pushed the type-hinting branch 2 times, most recently from 508c15d to ea9b71f Compare October 30, 2019 15:01

odolbeau approved these changes Oct 30, 2019

View reviewed changes

ker0x force-pushed the type-hinting branch 3 times, most recently from 8faa261 to 30a6079 Compare November 1, 2019 02:48

Add I/O type hinting, use null coalescing operator

c21280d

ker0x force-pushed the type-hinting branch from 30a6079 to c21280d Compare November 1, 2019 02:52

bocharsky-bw reviewed Nov 4, 2019

View reviewed changes

rvanlaak mentioned this pull request Nov 4, 2019

[WIP] Run PHPStan on level 2 #334

Closed

Fix setKey signature

083bff5

rvanlaak added 2 commits November 5, 2019 11:24

documentation fixes for phpstan lvl 2

1c30362

bug and codestyle fixes

58acf97

rvanlaak mentioned this pull request Nov 5, 2019

Raise phpstan to level 2 ker0x/symfony-translation-bundle#1

Merged

rvanlaak mentioned this pull request Nov 6, 2019

Fix codestyle after uptream release of cs-fixer #344

Merged

ker0x and others added 3 commits November 8, 2019 09:57

Merge pull request #1 from scribbr/type-hinting-phpstan-lvl2

36e278b

Raise phpstan to level 2

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

6e5d463

Merge remote-tracking branch 'composer/master' into type-hinting

bdf65ab

ker0x force-pushed the type-hinting branch from 55962bc to 0629945 Compare November 8, 2019 10:52

Fix CS

b794c18

ker0x force-pushed the type-hinting branch from 0629945 to b794c18 Compare November 8, 2019 10:59

rvanlaak approved these changes Nov 11, 2019

View reviewed changes

rvanlaak requested a review from Nyholm November 13, 2019 16:02

rvanlaak mentioned this pull request Nov 13, 2019

Added Symfony 4 compatibility and refactored code #345

Closed

bocharsky-bw approved these changes Nov 18, 2019

View reviewed changes

welcoMattic merged commit 8d3ead8 into php-translation:master Nov 27, 2019

odolbeau mentioned this pull request Jan 8, 2020

Method getRawData has been removed, but still has usage #343

Closed

ker0x deleted the type-hinting branch January 28, 2020 16:43

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

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

Uh oh!

Conversation

ker0x commented Oct 30, 2019

Uh oh!

bocharsky-bw 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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvanlaak commented Nov 4, 2019

Uh oh!

Nyholm commented Nov 4, 2019

Uh oh!

rvanlaak commented Nov 5, 2019

Uh oh!

Nyholm commented Nov 5, 2019

Uh oh!

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

Uh oh!

Uh oh!

ker0x commented Nov 5, 2019

Uh oh!

rvanlaak commented Nov 6, 2019

Uh oh!

rvanlaak commented Nov 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvanlaak commented Nov 13, 2019

Uh oh!

rvanlaak commented Nov 25, 2019

Uh oh!

bocharsky-bw commented Nov 27, 2019

Uh oh!

odolbeau commented Nov 27, 2019

Uh oh!

welcoMattic commented Nov 27, 2019

Uh oh!

Uh oh!

bocharsky-bw left a comment •

edited

Loading

rvanlaak commented Nov 5, 2019 •

edited

Loading