Mismatch between requirements

[BorisovskiP]

It is not possible to update to magento 2.4.0 community edition because your module requires phpstan/phpstan: ^0.12.24, while magento requires phpstan/phpstan: >=0.12.3 <=0.12.23 (https://packagist.org/packages/magento/community-edition).

[shochdoerfer]

Oh my that sucks big time ;(

Could you try to remove bitexpert/phpstan-magento before upgrading to Magento 2.4 and installing it afterwards again? You still might need to upgrade the phpstan/phpstan version. Not sure if this breaks Magento's internal phpstan setup, but I guess if you are using this module you might not care :)

[shochdoerfer]

@BorisovskiP were you able to get it running the way I described it?

[BorisovskiP]

@shochdoerfer as we have other modules that are still not compatible with 2.4.0, we are waiting on them to update their dependencies so we can proceed with the update. I will notify you when we have done this.

[BorisovskiP]

Removing and re-installing this module does not work as the requirements are still incompatible as stated in the initial description. We just updated the version constraint for phpstan/phpstan.

[sprankhub]

So the big question still is how this issue should be handled by default. I tend to vote for "use the PHPStan version Magento uses". I strongly believe a Magento package should be installable on a clean Magento installation without any issues. Hence, the PHPStan versions must not conflict.
Additionally, we could add a note in the Wiki, which says that "you may want to update the version of PHPStan to get better results" or something.

What do you think, @shochdoerfer?

[shochdoerfer]

Yeah, it kind of sucks to downgrade phpstan again but I am fully with you @sprankhub. It needs to be easy to install the module in any Magento environment and it would suck big time if the phpstan rules shipped with Magento would not work anymore after the phpstan upgrade. So there's only one choice here, sadly.

I don't think we can support multiple phpstan versions, especially not "latest" as usually there's lots of change happening. I'd rather only support the phpstan version that is shipped with the latest, stable Magento version.

[shochdoerfer]

@sprankhub feel free to prep a PR for this, I am way too busy these days to get this going ;(

[shochdoerfer]

Fixed in master, will prepare a new release soonish.

[hostep]

@BorisovskiP: can you answer these questions please?

1. Why do you need bitexpert/phpstan-magento as a direct dependency in a Magento project? What extra features does it provide beside the native implementation in Magento?
2. If you don't use it as a direct dependency in a Magento project, but as a dependency of a module, then I believe this is incorrect and it should be a dev-dependency, is that the case here maybe?

[shochdoerfer]

Following @hostep's remarks in #43 I think we have re-evaluate the approach taken and adapt.

The original goal was to allow people to easily upgrade their Magento 2.3 instance with the bitexpert/phpstan-magento dependency installed to 2.4.x - back then that was not possible to due the hard constraint that Magento has on the phpstan dependency. "Sadly" phpstan's different versions change quite rapidly which leads to the problem @hostep mentions in #43.

I would not want to restrict people using bitexpert/phpstan-magento "only" for module development. We also have the dependency in our Magento project and I would like to keep it as such. If that comes with cost of uninstalling our module & phpstan before upgrading to 2.4 and then reinstalling everything, that would be "ok" for me. If someone installs bitexpert/phpstan-magento on purpose for Magento 2.4, I would not care if the default phpstan configuration in Magento breaks or things fail. The upgrade vom 2.3 to 2.4 should only happen once in the lifespan of a Magento project, I assume.

Ideally, we could keep up with phpstan development and adapt more often than Magento does (or can). Or at least be a bit more independent from Magento's release cycles.

So my plan would be to release a bugfix release that would revert #43 and add a bit of documentation to describe the upgrade process when coming from Magento 2.3. What do you people think? @hostep @BorisovskiP @sprankhub @torhoehn - I would really get this right this time :)

[hostep]

Are there valid cases of making bitexpert/phpstan-magento a dependency in the composer.json file of an entire Magento project?

If yes, then we should indeed make it possible to install it in Magento 2.4, and then I would suggest the constraint to be changed to at least allow it to be installed with phpstan/phpstan 0.12.23, so that will actually work.
But this then conflicts with the installation instructions of bitexpert/phpstan-magento, because for the installation instructions to work, we need at least version 0.12.26 of phpstan. But than maybe the installation instructions on the readme needs to be changed specifically for usage as a direct dependency of a Magento project?

When you install bitexpert/phpstan-magento outside of a Magento project, we will still be able to use higher versions of phpstan as expected.

I also don't really understand why in Magento 2 they limit phpstan to 0.12.23 as the highest version, this was added in scope of adding support for Mysql 8, but makes little sense to me: magento/magento2@700de11
We could also try to send in a pull request to the magento2 repo, to try to increase the phpstan version allowed? Isn't phpstan developed with semantic versioning in mind? If a new breaking change is introduced the version would be increased to 0.13.0 I think?

[sprankhub]

Yes, there are definitely valid use cases for making bitexpert/phpstan-magento a dependency of an entire Magento project! Client-specific modules are usually developed either under app/code or under a local directory, which is then set up as a local Composer repository (our approach). We then check this whole directory with PHPStan. I think this should definitely be possible - and if I understood correctly, @shochdoerfer thinks the same.

I think changing the requirement to something like ^0.12.18 as suggested by @hostep in #43 makes sense. I do not see any disadvantage of doing this.

I also think a ticket should be created in the M2 repo to get rid of the upper PHPStan version constraint. Would anyone of you guys be willing to do so?

[hostep]

Ok, that makes sense @sprankhub !

I've opened a pull request on the M2 repo to allow >= 0.12.3 < 0.13.0 versions of phpstan to be installed: magento/magento2#30581

Unfortunately for this repo, we will need to change the requirements to the closest possible to the actual minimum required version, which seems to be 0.12.26, while still allowing it to be installed on Magento 2.4.x, so I would suggest: ^0.12.23?

[sprankhub]

Sounds reasonable to me, yes. Let us see what @shochdoerfer says :) Thanks @hostep!

[shochdoerfer]

Sorry, got hit hard by a migraine yesterday, not sure if this happened because of this issue or not :P

A bit busy today, will try to reply to you later...

[shochdoerfer]

Was hoping to get this sorted before my MageCONF 2020 presentation today but life got in my way.

Sadly we can't aim at ^0.12.23 if I am not mistaken, since the bootstrapFiles option that we currently rely on was introduced with 0.12.26. Not sure if I really would want to revert this behaviour since a few things have changed quite a bit in phpstan.

So I would aim at ^0.12.26 and add a remark in the readme that for an 2.4 upgrade one would need to uninstall and reinstall the module and for a fresh install the phpstan dependency version needs to be increased beforehand.

Or do we have another option that I currently not see?

[sprankhub]

AFAIK, the new bootstrapFiles parameter works pretty much the same as the old autoload_files parameter. And AFAIK, we currently only rely on this parameter in the README. Hence, the nicest approach IMHO would still be to use a ^0.12.23 dependency, revert to autoload_files in the README and add a note directly below that the parameter should be updated to bootstrapFiles if newer versions of PHPStan are used.

[shochdoerfer]

Right, I forgot about the autoload_files settings, I thought we had moved to the bootstrapFiles setting right from the default Composer autoloader configuration we had in place.

Then I think it boils down to 2 options:

1. Add a constraint like >=0.12.23 <=0.12.25 and add the autoload_files hint to the docs. Since autoload_files got deprecated in 0.12.26 users will get a notice while running phpstan that the configuration setting is deprecated. Would love to avoid that displaying to the users to not confuse them. This way we'd be fully compatible with M2.4
2. Ignore the M2.4 compatibility and set the constraint to ^0.12.26 and add a hint to the docs describing what to do when updating/installing M2.4

For now, I am fine with option 1. Depending on Magento's reasons for the hard version limit, future versions might not be compatible with Magento core. PHPStan is moving fast and I would at least try to keep up to have all the goodness available :)

[hostep]

I've just tried upgrading from version 0.3.0 to 0.4.0 of bitexpert/phpstan-magento and downgrading from 0.12.50 to 0.12.23 of phpstan/phpstan on my own project (not inside a Magento project) and the only thing that needed to be done to get it working again is to change bootstrapFiles to autoload_files in the phpstan.neon file configuration.
So this means the functionality of bitexpert/phpstan-magento works perfectly fine across all phpstan versions between 0.12.23 and 0.12.50. The only difference is in how you configure it in your phstan.neon file.

So I'm more in favor of option 2, but maybe we can get a compromise by still using ^0.12.23?

We'd then just have to put 2 different statements in the installation section of the readme file, one using autoload_files and another for bootstrapFiles depending on if you use a version < 0.12.26 of phpstan/phpstan or >= 0.12.26

So I agree with @sprankhub here.

The other option is actually going for suggestion number 2, because the constraint on phpstan/phpstan in a Magento project is done in the root composer.json file of the project. So this means users are in control of this dependency and it's not defined by some dependency in a Magento module of a Magento project. This can also be explained in the readme file how users can change that constraint to make it work with bitexpert/phpstan-magento

I really dislike option 1, because it would mean we can't use the latest and greatest of phpstan/phpstan.

[shochdoerfer]

Ok, so that's 1,75 votes for option 2, what about you @sprankhub?

[sprankhub]

TBH, I think you ignore my proposal ;-)

Hence, the nicest approach IMHO would still be to use a ^0.12.23 dependency, revert to autoload_files in the README and add a note directly below that the parameter should be updated to bootstrapFiles if newer versions of PHPStan are used.

I do not see any disadvantage of this. Only advantages:

• No upper version constraint means people can use higher versions of PHPStan.
• Installable by default in Magento 2.4.
• If people read the README correctly and use the right parameter autoload_files / bootstrapFiles depending on the version of PHPStan they use, no deprecation warning.

Tell me if I miss anything.

[shochdoerfer]

@sprankhub then let's go with the "nicest approach" that I may or may not have ignored :)

Any takers for the PR?

Are you all cool with releasing this as a patch release?

[hostep]

PR created: #44

I'm fine with a patch release!

[shochdoerfer]

Patch relase 0.4.1 published. Thanks a lot!