Add composer-require-checker to the CI build

[mmenozzi]

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	#10648
License	MIT

[mmenozzi]

Hi guys,
IMHO this composer-require-checker thing is ready for review.
Would you like me to squash in a single commit?

[mmenozzi]

@pamil @lchrusciel @Zales0123 what do you think?

[Zales0123]

At first glance, it looks good 👍 And I like the idea of having no soft dependencies in the composer. But still, I would like to see @pamil and @lchrusciel opinions 🚀

[lchrusciel]

I hope, we won't have too many problems with maintenance of it (I'm worried mostly about dynamic changes in composer,json file). Other then that, I'm all in.

Maybe it would be better to add it to only one of the jobs?

[Zales0123]

What is the status of this PR? It looks good for me, to give it a try 👌 cc @pamil @lchrusciel

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

[vvasiloi]

Hey @mmenozzi, how do you fill about reviving this?

[mmenozzi]

Hey @mmenozzi, how do you fill about reviving this?

Hi @vvasiloi, I'd be happy to revive this if Sylius core team agree to merge this as soon as it will be ready.
Reviving this PR now, after 2 years, basically means that I need to start again from scratch and I'd like to be sure that it will be merged then.

[vvasiloi]

Hey @mmenozzi, you have my full support and I can assure you that the core team won't let you down on this one the second time.

[mmenozzi]

Ok @vvasiloi thank you! I'll do my best to get this work done asap.

[mmenozzi]

Hi @vvasiloi, currently if someone uses the Sylius\Bundle\CoreBundle\Application\Kernel will get errors because many bundles registered there (for example the Doctrine\Bundle\DoctrineCacheBundle\DoctrineCacheBundle) are not required anymore.

How you'd handle that?

[vvasiloi]

@mmenozzi no one should use that class in their application, it was deprecated in Sylius 1.3.

[mmenozzi]

@mmenozzi no one should use that class in their application, it was deprecated in Sylius 1.3.

Yes @vvasiloi I see that is deprecated since Sylius 1.3. But deprecated does not mean removed. Deprecated means that we should support it until Sylius 2.0. Instead that class does not work anymore since someone removed required packages from composer.json (for example the DoctrineCacheBundle).

1. If we really want to support that old Kernel class we should re-add those dependencies to the composer.json file.
2. Or we could decide that is not worth because, in the end, its support already ended several releases ago when someone removed required dependencies. If this is the case I'd completely remove that old Kernel class.
3. The last, and more messy option, is to keep that Kernel class without adding required dependencies to the composer.json and whitelist them in the ComposerRequireChecker configuration.

I'd go with the option number 2.

[vvasiloi]

@mmenozzi Tough choice. I remembered now that Sylius broke that compatibility in 1.7: #12463.
Re-adding some of those dependencies will not be possible, because we're dropping support for Symfony 4:

• DoctrineCacheBundle was deprecated and doesn't support Symfony 5.
• SonataCoreBundle was deprecated and doesn't support Symfony 5.
• Symfony WebServerBundle was deprecated, but supports Symfony 5.
• SonataIntlBundle is still maintained and supports Symfony 5 and 6.
• DoctrineFixturesBundle is still maintained and supports Symfony 5 and 6.

The Sylius\Bundle\CoreBundle\Application\Kernel class also uses Symfony\Component\HttpKernel\Kernel::getRootDir, which was deprecated in Symfony 4 and removed in Symfony 5.

I don't think it's worth supporting that upgrade path anymore, especially since it was unfortunately broken long ago.
The damage is already done, so we can probably remove it already...
In the current state, I think it's virtually impossible to upgrade past Sylius 1.7 without upgrading the directory structure and removing the use of Sylius\Bundle\CoreBundle\Application\Kernel or adding those dependencies to the end project's requirements.
@stefandoorn can probably attest to that.

We might be able to bring back the BC for this class in Sylius 1.10 and 1.11, because those still support Symfony 4, but again, I don't think it's worth it.

Anyway, regardless of which path we choose, it should not be part of this PR.

@Sylius/core-team I would love to see more opinions on this issue.

[Roshyo]

I would also go for option 2. As you said, it has been broken for a while now, and upgrading to 5.x/6.x should be the way.

IMO it is okay to ask a few changes in a project when upgrading if there are valid reasons. In that case, as you said again, no-one should use this Kernel, so "forcing" them to make the switch is fine as long as it is documented in upgrade files.

[mmenozzi]

Anyway, regardless of which path we choose, it should not be part of this PR.

Ok @vvasiloi but this leads to option 3 for this PR. I don't like it that much because every symbol we whitelist in the ComposerRequireChecker configuration is global and not only for a certain usage. So we have to remember that there are those "cleanup PRs" to be made in the future.

I wonder if it would be better to delete that old Kernel class in this PR.

[vvasiloi]

@mmenozzi True, it's going to be a bit messy. However, going from no dependency checks at all to checking most dependencies, but with quite a few exceptions is still good. We can open the follow-up PR(s) right after merging this.
I'm just worried that this PR will get stale again because of those tough decisions and I just want it to move forward ASAP and sort out everything else afterwards.

[lchrusciel]

Hey folks. Thanks a lot for comments and pushing this topic forward. I can agree, that the current solution is the best to move this topic forward and then do it case by case. At the current state, I would love to merge it, but the build is red.

[mmenozzi]

Yes @lchrusciel it's red because it's still work in progress.

[mmenozzi]

Hi @vvasiloi and @lchrusciel,
I think that this PR is now ready for merge.

I created separate commits for each issue that, in my opinion, must be fixed in separate PR. In those commits comment you can find more information about the issue, they are the following:

• 30c42ff: there is an old Kernel class that can be removed.
• e09551c: the Sylius\Bundle\CoreBundle\EventListener\CircularDependencyBreakingExceptionListener uses symfony/http-kernel symbols from version 4 but the support for Symfony 4 has been removed in Sylius. I think that this issue is the most critical because currently if this class is used will throw a fatal error.
• 5d4765e: hwi/oauth-bundle is an optional dependency (see here). That's ok but this forced me to put its symbols in the ComposerRequireChecker whitelist. One way to handle that is to create a separate plugin that could be installed only if someone wants to use the Facebook login.

Remember that for ComposerRequireChecker every whitelisted symbol is global and not limited to a certain usage. For this reason there should be less whitelisted symbol as possible. Think for example to the hwi/oauth-bundle symbols. If someone uses it during the development of a new Sylius core feature without putting the dependency in the require section of the composer.json file, ComposerRequireChecker will not trigger any error because those symbols are whitelisted. This leads to bugs.

After this PR will be merged I will create 3 separate issues to track down those problems.

[vvasiloi]

Thank you, @mmenozzi! Great work! 🥇
It's now up to @lchrusciel to merge.

[mmenozzi]

Thank you @vvasiloi!
@lchrusciel the build is green now, you can merge it! 😉

[lchrusciel]

Hey Manuele!

Thanks a lot for your contribution! I feel sorry, that we keep you waiting for over two years, but luckily Victor bring this topic again to light. After all this time, lest finally merge it!

[mmenozzi]

Yeah! Thank you guys! 🎉