Skip to content

Update dev tools #785

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

Closed
wants to merge 1 commit into from
Closed

Conversation

jack-worman
Copy link
Contributor

@jack-worman jack-worman commented Feb 12, 2025

Moved dev tools to their own composer.json, so that we can get the newest versions while still supporting PHP 7.2 for the main composer.json.

Styling changes are due to updated php-cs-fixer, no rules were changed.

@jack-worman jack-worman force-pushed the Phpstan-fixes branch 2 times, most recently from f6f2388 to 6eb3168 Compare February 12, 2025 02:22
@jack-worman jack-worman marked this pull request as ready for review February 12, 2025 02:29
@DannyvdSluijs
Copy link
Collaborator

@jack-worman I really like the effort. But I'm struggling a bit with the PR. My first gut reaction is that the PR seems rather large (in number of files/lines) but with only a single commit. Beside the code changes it is turning things quite a bit upside down with the tools sub-project.
Moving tools into a complete subproject on it's own is something I have not seen before and might confuse other devs as well as it is non-standard.

Changes related to friendsofphp/php-cs-fixer also raises it to 3.68.5 which requires PHP 7.4. There is currently a bug #598 with a PR #783 to upgrade it to a version compatible to PHP 7.2. The reasoning behind that is that any dev running PHP 7.2 can contribute to the project.

But I'm also trying to understand it from your perspective. Could you perhaps elaborate on the problems you ran into that resulted in this PR? Perhaps we can find an alternative solution to this?

@jack-worman
Copy link
Contributor Author

jack-worman commented Feb 12, 2025

My first gut reaction is that the PR seems rather large (in number of files/lines) but with only a single commit.

I can split it into 3 commits if you wish:

  1. Moving php-cs-fixer to tools/composer.json, apply fixes, and updating CI
  2. Moving phpstan to tools/composer.json, running baseline, and updating CI
  3. Updates to the lint CI to make it run on all supported PHP versions

Moving tools into a complete subproject on it's own is something I have not seen before and might confuse other devs as well as it is non-standard.

I don't believe it too be uncommon for a project to set up their dev tools in this fashion. For instance, PHPStan does something similar: https://github.com/phpstan/phpstan-src/blob/2.1.x/build-cs/composer.json

I would be happy to add some instructions to CONTRIBUTING.md to help alleviate any confusion for future contributors.

Could you perhaps elaborate on the problems you ran into that resulted in this PR? Perhaps we can find an alternative solution to this?

In previous PR's,

  • I could not run php-cs-fixer locally with PHP 8.4 because it performed fixes the CI didn't apply.
  • PHPStan would generate different error messages because of my local version of PHP being different than CI's. I fixed this with setting phpVersion: 70200 in phpstan.neon, but this isn't the optimal solution. In newer versions of PHPStan, you can set a range of version with:
parameters:
    phpVersion:
        min: 70200
        max: 80499

This allows for more accurate analysis for the project.

And in general, it is much more enjoyable to work in a project that allows using modern tooling.

@jack-worman
Copy link
Contributor Author

The reasoning behind that is that any dev running PHP 7.2 can contribute to the project.

We should still support PHP 7.2 because it may be hard for people to update their projects to support newer PHP versions.

But the individual should have no problem installing a supported versions of PHP.

@DannyvdSluijs
Copy link
Collaborator

I've been giving this some thoughts over the past couple of days. Although I'm still somewhat conflicted on my decision I'm making it anyway. The end state you're trying to get to seems great from a technical perspective, and I understand your perspective. But it is adding additional cognitive load for the other developers as it deviates from the (expected) standard. In addition to the cognitive load the changes adds to the workload of the maintainers, which is me and some efforts from @erayd at this moment in time. So at this point in time the PR will not be accepted.

Several months ago this project was more dead than alive and we are still trying to restore from that, picking up reported issues, reviewing PR's and updating to support modern versions of PHP and JSON Schema Drafts. My preference is to put efforts into those things.

I do believe your contributions in terms of improving the codebase based on static analysis findings is very helpful to the project. I'm hoping you're still motivated to keep doing that even though this PR won't be accepted. Flexing the version constraints of the require-dev section would be a viable option to be able to run more up to date versions of the tooling we use for the developer experience.

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.

2 participants