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

API: Upgrade to phpstan 0.12 #23

Closed
wants to merge 3 commits into from
Closed

API: Upgrade to phpstan 0.12 #23

wants to merge 3 commits into from

Conversation

sminnee
Copy link
Contributor

@sminnee sminnee commented Feb 26, 2021

Unfortunately retaining 0.11 support isn’t feasible but that seems like
an acceptable tradeoff.

Note that some of the type-hinting of DataLists will now be doable with
phpstan’s template features - we can type hint DataList for
example, and we can commit some @phpstan-return clauses to the core
framework to support this.

Sam Minnee added 3 commits February 26, 2021 17:47
Unfortunately retaining 0.11 support isn’t feasible but that seems like
an acceptable tradeoff.

Note that some of the type-hinting of DataLists will now be doable with
phpstan’s template features - we can type hint DataList<Member> for
example, and we can commit some @phpstan-return clauses to the core
framework to support this.
@mleutenegger
Copy link
Contributor

@nglasl or any other maintainer, any chance to get this merged? It would be really nice to be able to upgrade to phpstan 0.12

@mleutenegger
Copy link
Contributor

mleutenegger commented Mar 8, 2022

@sminnee I noticed that the suggested version in the composer.json file is still 0.11:

"suggest": {
"phpstan/phpstan-shim": "~0.11.0"
},

I think this should be changed to "phpstan/phpstan": "~0.12.0".

On a sidenote, I personally think that it would be beneficial to require phpstan/phpstan in the dependencies directly. The main arguments for this would be:

  • The required version of phpstan is somewhat bound to the version of this module (eg. ^4.0 can only be used with phpstan 0.11, while the next version would be using phpstan 0.12 exclusively, the following one 1.0 and so on). This leads to basically having to upgrade this module alongside phpstan or vice-versa whenever one or the other updates.
  • when using things like dependabot, updating phpstan is basically impossible to do automatically, as manual work will always have to be done to upgrade this module or phpstan.
  • This module should be installed as a dev-dependency, so there would not be any pollution of production environments with the phpstan binary
  • There is a better developer experience in only having to care about updating this module and not having to chase phpstan updates separately

Curious what you would think about this.

@marwan38
Copy link

@mleutenegger PHPStan is currently at version 1.4.10.. FYI

@mleutenegger
Copy link
Contributor

mleutenegger commented Mar 16, 2022

PHPStan is currently at version 1.4.10.. FYI

@marwan38 Even more reason to get this working with newer versions and keep this maintained. As far as my comment was concerned, I was referring to the target version of this PR.

It would be a good idea to jump to the newest version, but this will probably require some additional work. Will have to check that and most likely add a new PR...

@sminnee
Copy link
Contributor Author

sminnee commented Mar 18, 2022

@nglasl are you still using this much at Symbiote? If not, would it make sense to publish a fork? Perhaps @mleutenegger would be interested in maintaining such a fork?

@marwan38
Copy link

marwan38 commented Mar 19, 2022

@sminnee I'm trying to develop this extension locally, but, I can't get it to run. Do you mind sharing how you set up this repo to be able to develop on it?

EDIT: Going to try cloning into vendor folder and requiring it by file

@mleutenegger
Copy link
Contributor

@sminnee Actually, I already thought about creating and publishing a fork under the syntro/... namespace after opening an issue here asking wether this module is still actively maintained (which seems to not be the case ... 😔 ). As the module is basically part of the test pipeline of every client project and I personally like the idea of having a static analysis tool working with Silverstripe, I am somewhat interested in getting this back to an actively maintained state.

It would take some lifting to get it to work with the newest PHPStan version (especially https://phpstan.org/developing-extensions/always-read-written-properties is not happy with properties being used as config values).

@nglasl
Copy link
Contributor

nglasl commented Mar 20, 2022

Sorry guys, I've moved on from Silverstripe development and I'm no longer a maintainer, however as I recall we weren't actively using this outside a single project.

@mleutenegger
Copy link
Contributor

Perhaps @mleutenegger would be interested in maintaining such a fork?

@sminnee I have now created such a fork, as I was troubled with outdated dependencies & conflichts resolving them. Will start updating projects to use this fork in the near future.

It is compatible with the newest version of PHPStan and installs it as a dependency (so no more suggest) and so far seems to work fine. If interested, I would be grateful if you could give it a spin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants