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

Use same constraint as latest Magento release #43

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

torhoehn
Copy link
Contributor

As described in #42 if I got you right. :)

@shochdoerfer shochdoerfer added this to the 0.4.0 milestone Oct 16, 2020
@shochdoerfer
Copy link
Member

Thanks @torhoehn! Great contribution :) Very much appreciated!

@shochdoerfer shochdoerfer merged commit e80ca17 into bitExpert:master Oct 16, 2020
@torhoehn torhoehn deleted the feature/phpstan-version branch October 16, 2020 07:58
@hostep
Copy link
Contributor

hostep commented Oct 20, 2020

I don't think this was a good idea.

Are we sure that this package is compatible with phpstan 0.12.3? According to c4ac75a it probably needs a minimum version of 0.12.18? But I might be wrong.

Then, by constraining it to not be updated to versions higher than 0.12.23 you're making it difficult for people who do actually want to use versions higher than this. I would remove this upper constraint because:

  1. Using bitexpert/phpstan-magento directly in a Magento project will still work, composer will make sure phpstan is at version 0.12.23 even if bitexpert/phpstan-magento allows higher versions
  2. People using bitexpert/phpstan-magento outside of a Magento project (like I do), will still get the latest phpstan versions with all the new features and bugfixes. I am running version 0.12.50 of phpstan without any issues using version 0.3.0 of bitexpert/phpstan-magento

If it turns out this project needs at least version 0.12.18, I suggest we change the constraint to "phpstan/phpstan": "^0.12.18. But if it is actually compatible with 0.12.3, then we should change it to "phpstan/phpstan": "^0.12.3.

Thoughts?


(The release of 0.4.0 should also mention #43 instead of #40 on the first line 😉 )

@hostep
Copy link
Contributor

hostep commented Oct 20, 2020

Thinking about this some more, why do you even want to install bitexpert/phpstan-magento as a direct dependency of a Magento project, don't they already handle phpstan natively? What extra features do you get by doing this?

@hostep
Copy link
Contributor

hostep commented Oct 20, 2020

Also: the installation instructions mention to use bootstrapFiles, but that feature was introduced in phpstan version 0.12.26.

So this PR makes no sense. I think everybody got confused by an invalid request in #42, no?

@shochdoerfer
Copy link
Member

@hostep thank you very much for your input and big apologies for not thinking things through completely. I would say the idea came with good intentions but reality hits us hard. I'll open #42 again to have the discussion there on how to solve things.

Just wondering: What is your use case for using bitexpert/phpstan-magento outside of a Magento project? Are you talking about module development or about developing something entirely different?

@hostep
Copy link
Contributor

hostep commented Oct 21, 2020

No worries! 🙂

Yes, Magento module development, but not using an entire Magento project. This tools is used for static code analysis and not integration or functional testing. So we only need some part of the source code of some Magento modules and not an entire Magento project to be able to run the static tests.

https://github.com/baldwin-agency/magento2-module-url-data-integrity-checker is the project I'm talking about.

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.

3 participants