Skip to content
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: upgrade to PHP 8.0 with rector #6923

Closed
wants to merge 8 commits into from

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 29, 2022

Needs #6922

Description
See #6921

Applied rules:

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the 4.3 label Nov 29, 2022
@kenjis kenjis marked this pull request as draft November 29, 2022 01:55
@@ -123,7 +125,7 @@ public function getClassname(string $file): string
continue;
}

if ((isset($tokens[$i - 2][1]) && ($tokens[$i - 2][1] === 'phpnamespace' || $tokens[$i - 2][1] === 'namespace')) || ($dlm && $tokens[$i - 1][0] === T_NS_SEPARATOR && $token[0] === T_STRING)) {
if ((isset($tokens[$i - 2][1]) && ($tokens[$i - 2][1] === 'phpnamespace' || $tokens[$i - 2][1] === 'namespace')) || ($dlm && $tokens[$i - 1][0] === T_NS_SEPARATOR && $token->is(T_STRING))) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@samsonasik
This code causes an error.

Running PHPStan...
Note: Using configuration file .../CodeIgniter4/phpstan.neon.dist.
Error thrown in .../CodeIgniter4/system/Autoloader/FileLocator.php on line 128 while loading bootstrap file .../CodeIgniter4/phpstan-bootstrap.php: Cannot use object of type PhpToken as array

Copy link
Member

Choose a reason for hiding this comment

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

Possibly bug on TokenGetAllToObjectRector

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is a bug on TokenGetAllToObjectRector.
The $tokens in the original code is just an array of array, and used as $tokens[$i - 2][1].
But $tokens in the refactored code is an array of PhpToken object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Skipped the rule.

@kenjis kenjis added the refactor Pull requests that refactor code label Nov 29, 2022
@samsonasik
Copy link
Member

Cherry-picking rule by rule per-PR from config

https://github.com/rectorphp/rector-src/blob/2b35d2e75ea03ffe643c012485952ec6df3ea2ba/config/set/php80.php#L34

can be a way to make easier to catch breaking changes, so smallest PR will be easy to merge

@@ -630,7 +626,7 @@ public function toHeaderString(): string
/**
* {@inheritDoc}
*/
public function __toString()
public function __toString(): string
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this a breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

No, __toString is special, it keep working even child doesn't has return type

@kenjis kenjis changed the title refactor: upgrade to PHP 8.0 with rector refactor: upgrade to PHP 8.0 with rector (1) Nov 29, 2022
@kenjis kenjis changed the title refactor: upgrade to PHP 8.0 with rector (1) refactor: upgrade to PHP 8.0 with rector Nov 29, 2022
@kenjis
Copy link
Member Author

kenjis commented Nov 29, 2022

It seems difficult to apply ClasPropertyAssignToConstructorPromotionRector.
Some classes will be broken due to the existing type mismatches.

E.g.,

Fatal error: Type of CodeIgniter\HTTP\IncomingRequest::$uri must be ?CodeIgniter\HTTP\URI (as in class CodeIgniter\HTTP\OutgoingRequest) in .../CodeIgniter4/system/HTTP/IncomingRequest.php on line 45

@samsonasik
Copy link
Member

That can posibly due to autoload overlapped, the tweak may be needed https://github.com/rectorphp/rector/blob/main/docs/static_reflection_and_autoload.md

@kenjis
Copy link
Member Author

kenjis commented Nov 29, 2022

Probably due to property type mismatch between parent class and child class.
Fixing the type declaration might result in BC, so I skipped them for now.
See #6924

@kenjis kenjis added 4.4 and removed 4.3 labels Nov 29, 2022
@MGatner
Copy link
Member

MGatner commented Dec 1, 2022

I think the small approach is good. I will also note that I've found CS Fixer to be a great companion tool for Rector during the upgrades to make sure the results are still compliant with our style guide.

@kenjis kenjis deleted the branch codeigniter4:4.3 January 10, 2023 06:36
@kenjis kenjis closed this Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants