Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Nov 27, 2019

Conversation

ker0x
Copy link
Contributor

@ker0x ker0x commented Oct 30, 2019

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

@ker0x ker0x force-pushed the type-hinting branch 2 times, most recently from 508c15d to ea9b71f Compare October 30, 2019 15:01
@ker0x ker0x force-pushed the type-hinting branch 3 times, most recently from 8faa261 to 30a6079 Compare November 1, 2019 02:48
Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{
$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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

/**
* @param string $name
* @param string|string[]|null $name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

rvanlaak commented Nov 4, 2019

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
Copy link
Member

Nyholm commented Nov 4, 2019

Wohoo. Thank you!

@rvanlaak
Copy link
Member

rvanlaak commented Nov 5, 2019

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

@Nyholm
Copy link
Member

Nyholm commented Nov 5, 2019

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
Copy link
Member

rvanlaak commented Nov 5, 2019

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
Copy link
Contributor Author

ker0x commented Nov 5, 2019

@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
Copy link
Member

rvanlaak commented Nov 6, 2019

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

@rvanlaak
Copy link
Member

rvanlaak commented Nov 8, 2019

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 ;-)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be fixing a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rvanlaak rvanlaak requested a review from Nyholm November 13, 2019 16:02
@rvanlaak
Copy link
Member

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

@rvanlaak
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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 😉

@welcoMattic welcoMattic merged commit 8d3ead8 into php-translation:master Nov 27, 2019
@ker0x ker0x deleted the type-hinting branch January 28, 2020 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants