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: Migrate Neos.Fusion/Core to phpstan level 8 (Neos 8.3) #4844

Merged
merged 1 commit into from Feb 2, 2024

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jan 21, 2024

Requires: #4842

Only files in Neos.Fusion/Classes/Core have been migrated, as they contain relatively new code like the parser.

Two kind of errors have been partially ignored from the level 8

For one: "no value type specified in iterable type array" as adding array<int|string, mixed> would just bloat this commit up. In these files we are mostly dealing with the fusion configuration which is highly dynamic:

  • Neos.Fusion/Classes/Core/Runtime.php
  • Neos.Fusion/Classes/Core/Cache/RuntimeContentCache.php
  • Neos.Fusion/Classes/Core/Cache/ContentCache.php
  • Neos.Fusion/Classes/Core/Cache/CacheSegmentParser.php
  • Neos.Fusion/Classes/Core/RuntimeConfiguration.php
  • Neos.Fusion/Classes/Core/ObjectTreeParser/MergedArrayTreeVisitor.php
  • Neos.Fusion/Classes/Core/ObjectTreeParser/MergedArrayTree.php

And "has no return type specified" was noticed a lot in the ast visiting code. But i plan to refactor this instead to something better type- and readable in general:

  • Neos.Fusion/Classes/Core/ObjectTreeParser/MergedArrayTreeVisitor.php
  • Neos.Fusion/Classes/Core/ObjectTreeParser/AstNodeVisitorInterface.php
  • Neos.Fusion/Classes/Core/ObjectTreeParser/Ast/*

The phpstan config i used for local development & testing:

local phpstan config

parameters:
    level: 8
    ignoreErrors:
        -
          message: '#no value type specified in iterable type array.$#'
          paths:
            - Neos.Fusion/Classes/Core/Runtime.php
            - Neos.Fusion/Classes/Core/Cache/RuntimeContentCache.php
            - Neos.Fusion/Classes/Core/Cache/ContentCache.php
            - Neos.Fusion/Classes/Core/Cache/CacheSegmentParser.php
            - Neos.Fusion/Classes/Core/RuntimeConfiguration.php
            - Neos.Fusion/Classes/Core/ObjectTreeParser/MergedArrayTreeVisitor.php
            - Neos.Fusion/Classes/Core/ObjectTreeParser/MergedArrayTree.php
        -
          message: '#has no return type specified.$#'
          paths:
            - Neos.Fusion/Classes/Core/ObjectTreeParser/MergedArrayTreeVisitor.php
            - Neos.Fusion/Classes/Core/ObjectTreeParser/AstNodeVisitorInterface.php
            - Neos.Fusion/Classes/Core/ObjectTreeParser/Ast
    paths:
        - Neos.Fusion/Classes/Core
    bootstrapFiles:
         - ../Framework/bootstrap-phpstan.php

As phpstan doesnt allow to configure multiple levels by path, and i dont want to have to burden running phpstan multiple times, i decided against actually linting the migrated Neos.Fusion/Core code in ci for Neos 8.3. On the neos 9 branch this we will activate the full linting power though!

@mhsdesign mhsdesign marked this pull request as draft January 24, 2024 15:11
@mhsdesign
Copy link
Member Author

depends on neos/flow-development-collection#3274

Only files in `Neos.Fusion/Classes/Core` have been migrated, as they contain relatively new code like the parser.

Two kind of errors have been partially ignored from the level 8

For one: "no value type specified in iterable type array" as adding `array<int|string, mixed>` would just bloat this commit up. In these files we are mostly dealing with the fusion configuration which is highly dynamic:

- Neos.Fusion/Classes/Core/Runtime.php
- Neos.Fusion/Classes/Core/Cache/RuntimeContentCache.php
- Neos.Fusion/Classes/Core/Cache/ContentCache.php
- Neos.Fusion/Classes/Core/Cache/CacheSegmentParser.php
- Neos.Fusion/Classes/Core/RuntimeConfiguration.php
- Neos.Fusion/Classes/Core/ObjectTreeParser/MergedArrayTreeVisitor.php
- Neos.Fusion/Classes/Core/ObjectTreeParser/MergedArrayTree.php

And "has no return type specified" was noticed a lot in the ast visiting code. But i plan to refactor this instead to something better type able:

- Neos.Fusion/Classes/Core/ObjectTreeParser/MergedArrayTreeVisitor.php
- Neos.Fusion/Classes/Core/ObjectTreeParser/AstNodeVisitorInterface.php
- Neos.Fusion/Classes/Core/ObjectTreeParser/Ast
@mhsdesign mhsdesign marked this pull request as ready for review January 24, 2024 20:38
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.

Looks good in general. But you've changed a lot of method signatures (return types and argument types). I'm not sure how to handle this, as this might be a BC, right? But on the other hand I assume they are internal. 🤷‍♂️

@mhsdesign
Copy link
Member Author

Yes i made sure to not add return types anywhere where people even remotely would use them. Everything in ObjectTreeParser contains the "new" parser and is internal, and not usable really anyhow from the outside.

Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Yeah seems fine to me, docblocks I don't care, and signatures are very few. I guess the main risk would be someone extending the exception handlers and stumbling over the void return, but I don't even see that

@mhsdesign
Copy link
Member Author

the exception handlers and stumbling over the void return

Thanks. Yes the fact that the return type of injectThrowableStorage is void should not influence anyone ^^

@mhsdesign mhsdesign merged commit 8bb558e into neos:8.3 Feb 2, 2024
9 checks passed
@mhsdesign mhsdesign deleted the task/neosFusionPhpStanLevel8 branch February 2, 2024 13:00
@mhsdesign
Copy link
Member Author

Fyi 116 errors left when linting the whole fusion package. Which will be ultimately the goal, but that can be done at another time ;)

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Feb 2, 2024
@mhsdesign
Copy link
Member Author

mhsdesign commented Feb 2, 2024

Followup on 9.0 to fix ci: #4870

Thanks for your reviews ❤️

mhsdesign added a commit that referenced this pull request Feb 2, 2024
…e-in-90

TASK: Followup #4844 lint Neos.Fusion/Core in 9.0
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