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

TASK: PhpStan level 1 for Neos 8.3 #4842

Merged
merged 3 commits into from Feb 2, 2024
Merged

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jan 18, 2024

Adds phpstan to the Neos 8.3 branch to all packages with a minimal level 1:

  • Neos.ContentRepository/Classes
  • Neos.Diff/Classes
  • Neos.Fusion.Afx/Classes
  • Neos.Fusion/Classes
  • Neos.Media.Browser/Classes
  • Neos.Media/Classes
  • Neos.Neos/Classes
  • Neos.NodeTypes.Form/Classes
  • Neos.SiteKickstarter/Classes

Errors caused by the back-port of the ESCR TraversableNodeInterface, have been directly ignored via ignoreErrors.
Other actually fixable errors have been fixed in a non breaking way. (Adjusting the doc types e.g.)

This should provide a little starting base for extending php stan level across all packages to higher levels in the upcoming 9.0 branch, where we actually use phpstan at level 8 for the following packages:

  • Neos.ContentGraph.DoctrineDbalAdapter/src
  • Neos.ContentGraph.PostgreSQLAdapter/src
  • Neos.ContentRepository.BehavioralTests/Classes
  • Neos.ContentRepository.Core/Classes
  • Neos.ContentRepository.Export/src
  • Neos.ContentRepository.LegacyNodeMigration/Classes
  • Neos.ContentRepository.NodeAccess/Classes
  • Neos.ContentRepository.StructureAdjustment/src
  • Neos.ContentRepository.TestSuite/Classes
  • Neos.ContentRepositoryRegistry/Classes
  • Neos.Neos/Classes (also exists in 8.3)
  • Neos.NodeTypes.Form/Classes (also exists in 8.3)

As Neos.Fusion didnt need to be adjusted for neos 9 i started to migrate this package to level 8 for neos 8.3 already to avoid too many upmerge conflicts:

#4844

More packages, if applicable, will follow.

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign mhsdesign changed the base branch from 9.0 to 8.3 January 18, 2024 20:32
@github-actions github-actions bot added 8.3 and removed 9.0 labels Jan 18, 2024
@mhsdesign mhsdesign changed the title TASK: PhpStan level 0 for Neos 8.3 TASK: PhpStan level 1 for Neos 8.3 Jan 19, 2024
@mhsdesign mhsdesign marked this pull request as ready for review January 19, 2024 11:48
Copy link
Member Author

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

Fyi

@@ -597,7 +597,7 @@ public function getGroupedOpcodes($context = 3)
];
}

if (!empty($group) && !(count($group) == 1 && $group[0][0] == 'equal')) {
if (!(count($group) == 1 && $group[0][0] == 'equal')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a bit tricky but phpstan was really stubborn that this check is obsolete and it 100% is.

$group is never empty because it is filled in the foreach above where the variable $opCodes always have at least one entry.

@@ -922,6 +922,7 @@ public function getProperty($propertyName, bool $returnNodesAsIdentifiers = fals
}

if (
/** @phpstan-ignore-next-line i will not touch this code */
Copy link
Member Author

Choose a reason for hiding this comment

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

The old Node::getProperty does uuuuuglly things :D And i dont want to appear in its git blame :D

@@ -168,6 +168,7 @@ public function setOptions($options)
*/
public static function createByConfiguration($modeName, array $configuration)
{
/** @phpstan-ignore-next-line */
Copy link
Member Author

Choose a reason for hiding this comment

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

the user interface mode will be refactored either way with neos 9 ^^

@@ -110,6 +110,7 @@ public function processRequest(ActionRequest $request, ActionResponse $response)
*/
protected function convertException(\Exception $exception)
{
$exceptionData = [];
Copy link
Member Author

Choose a reason for hiding this comment

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

line 146 could in a very unlikely case throw an Error because the variable is not initialised.

Also line 116 makes use of phps implicit variable array initialisation which is unexpected ^^

Copy link
Contributor

@dlubitz dlubitz left a comment

Choose a reason for hiding this comment

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

Some nitpicks! Thank you for taking care of code quality!

@mhsdesign mhsdesign merged commit 59478f8 into neos:8.3 Feb 2, 2024
8 checks passed
@mhsdesign mhsdesign deleted the task/phpStan83 branch February 2, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants