swarrot / SwarrotBundle

A symfony bundle for swarrot integration
MIT License
89 stars 59 forks source link

Upgrade to 1.8 on Symfony 3.4 #191

Closed noniagriconomie closed 4 years ago

noniagriconomie commented 4 years ago

Hi

On one of my project I tried to pass to https://packagist.org/packages/swarrot/swarrot-bundle#v1.8.0 (being on v1.6)

But I have a failing test (the test does nothing more that trying to get all services from the container)

1) Tests\ContainerTest::testGetService
TypeError: Argument 2 passed to Swarrot\SwarrotBundle\Processor\ServicesResetter\ServicesResetterProcessorConfigurator::__construct() must implement interface Symfony\Contracts\Service\ResetInterface, instance of Symfony\Component\HttpKernel\DependencyInjection\ServicesResetter given

Symfony\Contracts\Service\ResetInterface seems to be for sf 4.*

FYI I am on Symfony 3.4LTS with https://packagist.org/packages/swarrot/swarrot#v3.7.0

Thank you,

odolbeau commented 4 years ago

Hi @noniagriconomie

You're right, both the ServicesResetterProcessorConfigurator of this bundle and the ServicesResetterProcessor in swarrot/swarrot are not compatible with sf3.4.

You can still migrate to SwarrotBundle 1.8 if you don't use this processor.

If you want out of the box support of this reset behavior with sf 3.4, you can propose a PR to update the existing processor & processor configurator in both the lib & the bundle.

Tell me it it's clear for you?

noniagriconomie commented 4 years ago

@odolbeau it is clear thank you,

but shouldn't then the composer of both lib and bundle be updated in the require part to be sf4+ ?

I will for now stay on version 1.7 on this project, will upgrade when upgrading to sf4.4

thank you again

odolbeau commented 4 years ago

Do you have this problem as soon as you update the SwarrotBundle? It's strange cause as all processors are optional, you should not have this processors configurator called if you don't add services_resetter in your processors_stack.

odolbeau commented 4 years ago

Oh OK, your test is failing cause your trying the retrieve the service even if you don't use it... :/

noniagriconomie commented 4 years ago

Yep indeed, as the lint:container command is on 4.4, we do have a "bare test" that loop the container services that is why I found this

Maybe removing the require ^3.4 from composer is a good start IMO, or add dependencies to sf/contracts ?

odolbeau commented 4 years ago

The only thing which breaks is your test. :) Swarrot should works just fine with sf3.4 as soon as you don't use this processor.

Removing the sf3.4 support for an optional processor which is not compatible is not a good idea IMO.

The container:lint command only compile the container, it does not try to access to all declared services. Maybe you could update your test to do the same?

noniagriconomie commented 4 years ago

ok understood @odolbeau

We will stay on 1.7 until going symfony 4.4 as I wrote earlier Would like to avoid having "sleeping non callable code" that can break May I propose a PR for adding an hint here https://github.com/swarrot/swarrot/blob/master/src/Swarrot/Processor/ServicesResetter/ServicesResetterProcessor.php#L13 about requiring sf4+ or specific usage of sf/contratcs?

lets close for now I've got my answer, thank you :)