MyIntervals / emogrifier

Converts CSS styles into inline style attributes in your HTML code.
https://www.myintervals.com/emogrifier.php
MIT License
907 stars 154 forks source link

[TASK] Add a fixer rule to ensure a PHP 8.4 compatibility #1271

Closed oliverklee closed 3 months ago

oliverklee commented 3 months ago

Ensure that parameters that have a null default value also have null as part of their type declarations.

Also ensure a uniform notation for nullable types.

https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated

Fixes #1269

oliverklee commented 3 months ago

I think we need to run the fixer with all supported PHP versions and/or get them to sort that problem out. The execution environment should not affect behaviour. It's the target environment that matters.

That it out of scope for this PR. So please let's have a separate ticket for that and then have a separate PR for that.

oliverklee commented 3 months ago

I'm not completely sure that getting the same results for PHP-CS-Fixer for all supported PHP versions is feasible as the PHP parsing used by the tools seems to depend on the PHP version it runs on: https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/issues/2378

JakeQZ commented 3 months ago

I'm not completely sure that getting the same results for PHP-CS-Fixer for all supported PHP versions is feasible as the PHP parsing used by the tools seems to depend on the PHP version it runs on: PHP-CS-Fixer/PHP-CS-Fixer#2378

Thinking about this, I don't think it's a problem. If some of the rules require a later PHP version, they just simply won't be run. So a developer using an older supported PHP version might not get all fixes applied (or be told about things to fix until committing a PR to GitHub). But they wouldn't end up with anything trying to be 'unfixed'.

However, what I don't understand is that the documentation for nullable_type_declaration_for_default_null_value says it should be a configutation array with a key use_nullable_type_declaration whose default value is true, and that it is deprecated, and that deprecation is included in 3.59.3 which we are using. But without it, we don't get the default behaviour. I also don't understand how or why a setting the rule to the primitive value true (instead of the configuration array) also achieves the result of setting the configuration parameter to what is suppsed to be its default value anyway.

oliverklee commented 3 months ago

My best guesses are this:

JakeQZ commented 3 months ago

My best guesses are this:

* `true` means "enable this rule and use the default configuration".

* The documentation might either not always be up to date, or it might be for the development version, not for the released version.

I think the first seems to be the case (and sometimes the second may be when changes are yet to be released, though the documentation seems to be auto-generated from the actual Fixer classes, so I would expect it to be up-to-date).

Some rules have nothing to configure, and can just be enabled or disabled (disabled by default). Other rules have configuration options, which can be explicitly specified but also have default values. For such rules, true can be used instead of an array of configuration options, to enable the rule with the default options. (I don't know if any empty array, being 'falsy', would have the same effect as true.) The default configuration options only apply if the rule is enabled, which by default it is not.

I couldn't find this clearly stated anywhere in the docs.

I think I have been conflating default configuration options (when a rule is enabled) and default rule application (which is always 'not applied'), so may need to revisit #1264 - I accepted all removals of rules with configuration options where we simply provided true, since their specification didn't seem to be correct; but some of those we actually may want to retain.

In the case of nullable_type_declaration_for_default_null_value, I now realize (IIUC) that it's the configration option use_nullable_type_declaration that has been deprecated, not the rule itself!

So for this PR, the second change is correct. However the first change is in the wrong section, and since the configuration option is the default anyway, we could just use true instead of a configuration array.