-
Notifications
You must be signed in to change notification settings - Fork 93
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
Conversation
508c15d
to
ea9b71f
Compare
8faa261
to
30a6079
Compare
There was a problem hiding this 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?
Command/DeleteObsoleteCommand.php
Outdated
{ | ||
$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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Nice job! The change in #334 on raising the phpstan level could help you with what you're trying to achieve with this PR 👍 |
Wohoo. Thank you! |
How does everyone feel about removing all phpdoc that can get determined based on the typehints for methods without a description? |
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. |
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 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. |
@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 |
Created #344 so this PR can focus on adding type hints 👏 |
@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', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes because xliff_version
is of type scalarNode
not arrayNode
, https://github.com/php-translation/symfony-bundle/blob/master/DependencyInjection/Configuration.php#L164
Asked @Nyholm for a final review, so he can approve and then merge this PR 👍 |
This PR most likely is going to give conflicts once the smaller PRs would get merged. @bocharsky-bw @odolbeau shall we merge this? |
@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 👍 |
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 🎉 |
As the package require PHP >=
7.1
, I added i/o type hinting when possible and replace someisset()
call with the null coalescing operator