-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
refactor: PHP 8.0 constructor promotion #6924
refactor: PHP 8.0 constructor promotion #6924
Conversation
Applied rules: * StrEndsWithRector (https://wiki.php.net/rfc/add_str_starts_with_and_ends_with_functions) * StrStartsWithRector (https://wiki.php.net/rfc/add_str_starts_with_and_ends_with_functions) * StrContainsRector (https://externals.io/message/108562 php/php-src#5179)
…anual tweaks This may contains breaking changes.
This is a breaking chage: |
Yes, that seems need to be skipped |
$this->autoloader = $autoloader; | ||
public function __construct( | ||
protected Autoloader $autoloader | ||
) { |
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.
For example, if a dev extends FileLocator
and it has protected $autoloader;
,
this change is a breaking change.
https://3v4l.org/NhqDb
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.
There are possible options:
- make a note about property re-definition possible breaking change without add type
- wait to push to 4.4 only
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.
Another way;: make Rector extension to help user migrating to new definition.
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.
Removing type in the constructor makes no errors (I don't know why),
https://3v4l.org/2A85L
but I don't want to remove already declared types in constructors.
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.
Better way: make a patch to rector to:
- skip non-typed property for protected/public property, except define
INLINE_PUBLIC
config that allow bc break on non-final class, same like TypedPropertyRector way
That's just quick guessing, probably there is a better way.
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.
@kenjis here possibly the solution, I create PR to make it configurable INLINE_PUBLIC
default false:
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 seems the best approach to me. Glad your patch was already accepted!
@kenjis some rules like |
If we drop PHP 7.4 in v4.4, I would like to type missing public/protected properties with constructor promotion. |
Yes, I agree 👍 |
Needs #6923
Description
See #6921
Use constructor promotion.
This may contain breaking changes.
Checklist: