[Dotenv] - refactor(dotenv): improvements for php 8.1 support #46674
Conversation
carsonbot
changed the title
|
PHP Same regarding |
There was a problem hiding this comment.
We're OK with such PRs but they consume a significant amount of review time for a limited benefit. Of course, I'm OK to modernize the code, but only if the submitter is willing to learn from previous reviews (like the one I'm submitting now) to reduce the review time on next PRs.
| private $projectDirectory; | ||
|
|
||
| public function __construct(string $kernelEnvironment, string $projectDirectory) | ||
| public function __construct(private string $kernelEnvironment, private string $projectDirectory) |
There was a problem hiding this comment.
The convention in Symfony source code tends to be the usage of newline between arguments with CPP. Example:
IMO, this helps the reader to identify that it is a property declaration, since the visibility keyword private is more visible.
There was a problem hiding this comment.
Hi @Guikingone.
As you can tell from my code comments, I'm very skeptical about the added benefit of most of these automated changes.
History has taught us that even the slightest changes can break someone's application when depending on Symfony. On top of that, any batch process changing something adds some maintenance overhead (short term - reviewing all changes, and long term - resolving merge conflicts when merging 4.4 up to 6.2). Of course, it's fine for modernization to have some costs, but we have to find a good balance between the two.
For that reason, I think it's better to discuss and review things like this per type of change, instead of per component. From the changes in this PR, I'm leaning towards saying that only 2 changes add some value:
- Constructor property promotion, as this shouldn't change any API of the class (I hope/guess that this is true when reflecting the class as well?) and helps in modernizing the code/making it consistent.
- Using arrow functions to replace one line closures (especially the one's
use-ing vars)
I'm not sure if anything else really adds anything that compensates the costs.
| @@ -181,7 +177,7 @@ public function populate(array $values, bool $overrideExistingVars = false): voi | |||
| $loadedVars = array_flip(explode(',', $_SERVER['SYMFONY_DOTENV_VARS'] ?? $_ENV['SYMFONY_DOTENV_VARS'] ?? '')); | |||
|
|
|||
| foreach ($values as $name => $value) { | |||
| $notHttpName = !str_starts_with($name, 'HTTP_'); | |||
| $notHttpName = !str_starts_with((string) $name, 'HTTP_'); | |||
There was a problem hiding this comment.
Not needed. Array keys are either strings or ints, and both are accepted by str_starts_with() (as ints can be casted to strings).
| if (!is_file($this->getFilePath('.env')) && is_file($this->getFilePath('.env.dist'))) { | ||
| $files[] = '.env.dist'; | ||
| } else { | ||
| $files[] = '.env'; | ||
| } | ||
| $files[] = !is_file($this->getFilePath('.env')) && is_file($this->getFilePath('.env.dist')) ? '.env.dist' : '.env'; |
There was a problem hiding this comment.
This does not add anything and I would argue that it makes it harder to read. In general, not worth the change (and time of review and merge) imho
There was a problem hiding this comment.
$files[] = !is_file($this->getFilePath('.env')) && is_file($this->getFilePath('.env.dist'))
? '.env.dist'
: '.env';| @@ -89,6 +80,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int | |||
| return 0; | |||
| } | |||
|
|
|||
| /** | |||
| * @return array<int, mixed[]> | |||
There was a problem hiding this comment.
This does not add much over the : array return type. In general, Symfony favors less PHPdoc.
|
I'm going to close here for all the reasons explained already. |
Hi 👋🏻
As discussed in the discussion #46593, here's the first "improvement PR" about applying Rector automated rules to Symfony source code, the main goal is to enforce the usage of PHP
8.1features / improvements.To see the impact, I decided to apply them to
DotEnv(which is central in Sf now but relatively small compared to components likeFormorDependencyInjection).So, what's inside?
First, readonly attributes are now available, small improvements on types (return types added on private method, AFAIK, private methods are not concerned about BC breaks but it could reverted if needed) and extra conditions (that can turned into ternaries), short syntax functions (along with static usage).
Again, this first PR is small as the component is relatively easy to improve and quite small, a lot of things could be discussed if you wish.
Thanks again for the feedback and have a great day 🙂