Symfony 5 Controller dependency injection

[davidmpaz]

Hi,

thanks for this project. I would like to use it in a Symfony 5 project i have but i am finding some issues regarding package dependencies and also DI Container.

After a small test i wonder if i can introduce some more configuration inside the bundle. To understand what i mean, i had to do this bindings in my application services.yaml file:

    Payum\Bundle\PayumBundle\Controller\:
        resource: '../vendor/payum/payum-bundle/Controller/'
        tags: ['controller.service_arguments']
    Payum\Core\Registry\RegistryInterface:
        public: true
        alias: 'payum'

Can i put these also inside the bundle to reduce effort on integration later on in applications using it?

Could someone please check this PR? Provide feedback or merge it? /cc @Tetragramat, @bennukem, @joshbmarshall ?

I am trying to solve issue: https://github.com/Payum/PayumBundle/issues/507

Thanks in advance, David

[Tetragramat]

There are already 2 other PRs for this issue. See https://github.com/Payum/PayumBundle/pull/513 and https://github.com/Payum/PayumBundle/pull/509.

Before fixing this issue, those controller tests should be modified so they test that controller services are properly configured. Right now it's skipped. I'll look into this in next days.

Can i put these also inside the bundle to reduce effort on integration later on in applications using it?

Yes all services should be configured out of the box including controllers.

[davidmpaz]

Hi there,

thank for your comments.

There are already 2 other PRs for this issue. See #513 and #509.

I noticed now about those others. I am trying to find the common denominator on the 3 to find a solution that solves all approaches.

As i see @BoShurik says his PR is an alternative solution to your PR. In both of them the container is injected via setter method. I assume this is only done in order to get the payum instance from container later on. Am i correct here?

Is the above is the only reason for injecting the container, i think the solution proposed in this PR will serve better to Symfony best practices since it inject the Payum instance and avoid depending on the container to get it.

The only work missing here would be to move the configuration i already have in the application into the Bundle configuration.

What do you think?

Before fixing this issue, those controller tests should be modified so they test that controller services are properly configured. Right now it's skipped.

Could you please point me which are those, so i can take a look to them.

thanks in advance for checking this out. Best regards, David

[Tetragramat]

Could you please point me which are those, so i can take a look to them.

Functional tests of controller actions https://symfony.com/doc/4.4/testing.html#functional-tests If you have no idea how to make them then I'll do it arround tuesday.

[davidmpaz]

Hey, thanks for your message.

What i understood from here:

Before fixing this issue, those controller tests should be modified...

was that there exist some tests already. I wanted to know which ones needs to be modified to look at them.

I do know those Functional Tests in the documentation you sent. What i don't know is what to test already :). I think it is a good idea to wait for your additions as you suggested.

Best regards, David

[Tetragramat]

I'm sorry If my responses feel a bit chaotic. I was talking about controller tests you've modified. But after revieving other PR for this issue I've noticed that functional tests are separated in it's own directory. So instead of you should make functional tests in Tests/Functional/Controller/ dir. Same as in my PR https://github.com/Payum/PayumBundle/pull/509/files. You can copy and modify those.

[davidmpaz]

Hey 👋, no problem at all. Actually i should apologise with you since last week i had some time to invest on the project and i feel like i was spamming you with mentions on my comments. We are all busy and with little time, so it is fine.

I think i got a better insight now about the DI in the bundle and controllers. I did notice that in either case we need to configure the container in the controllers if we want to make some real functional tests, like WebTestCase.

Some how i played with (and i liked) the idea of some kind of "late binding". Like a flex recipe or alike that could add the configuration needed at install time to configure controllers and bind payum to RegistryInterface like i did for my application.

In any case, i did merge your PR and add some changes to allow the injection on constructor. I also did leave the Tests under: Tests/Controller in their place since they are plain Unit Tests, all is mocked there, they are not functional test really.

Best, David

[davidmpaz]

Hi @makasim, could we please merge this one?

thanks in advance regards, David

[dpfaffenbauer]

@makasim Can you please merge that? Thanks.

[pierredup]

Thank you @davidmpaz