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

Set the payum.builder service as a non-shared service #551

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

pierredup
Copy link
Member

The payum service is set to use the payum.builder as a factory for creating the instance. This means that whenever you inject the Payum class, it will use the PayumBuilder->getPayum() method to instantiate the class. The PayumBuilder class is also defined as a service in the container, so that all it's dependencies can be added.

By default, all services in Symfony are shared, which means you will always get the same instance when requesting a service. Since PayumBuilder is used to create all the dependencies for Payum, and it requires the Twig env which might register an extension that also depends on Payum, it creates a circular dependency. But PayumBuilder is only a factory, and you don't rely on it directly in your classes, so it doesn't cause any errors during compliation.

But what does happen, is the order in which the gateway config is added.

Firstly, the payum.builder is added to the container, then it loads some dependencies.

$container->services['payum.builder'] = $instance = new \Payum\Core\PayumBuilder();

// .... 

$instance->addCoreGatewayFactoryConfig(['twig.env' => ($container->privates['twig'] ?? $container->load('getTwigService'))]);
$instance->addCoreGatewayFactoryConfig(...);

In this case, the twig service is added to the core config before the other config. When the twig service is loaded and an extension is registered that uses the Payum class, the service container will again use the payum.builder factory to create the Payum instance. But this time the payum.builder is already defined in the service container, but doesn't have all the config set yet. So the Payum class will be created, and added to the container (because services are shared by default), which means it won't have all the necessary config added.

This PR sets the payum.builder as a non-shared service. This means that when creating a Payum instance, it will use the PayumBuilder as a factory, and if PayumBuilder requires a service that relies on Payum, it will create a new instance of PayumBuilder with all the required dependencies. Once a successful instance on Payum is created, it will be added on the container as the payum service, which will be re-used whenever the Payum class is required.

This might have a very slight performance penalty, since the PayumBuilder->getPayum method might be called multiple times per request (I think it might be at most 2 times), but I think it's acceptable since there are no heavy operation in that method, and it ensures that the Payum service can be injected anywhere without side effects.

Fixes #549

@what-the-diff
Copy link

what-the-diff bot commented Aug 15, 2023

PR Summary

  • Change of shared attribute in payum.builder service
    The setting of sharing the payum.builder service has been altered. Previously, this service was shared across the system. Now, each time a request is made for this service, a fresh, unique instance is generated instead, ensuring there is no risk of data overlaps or interruptions. This change increases system stability and reliability.

@joesenova
Copy link

I'm going to start the process again, as I did over the weekend, and see where, with this change, the code breaks.

Thanks @pierredup

@pierredup pierredup merged commit a10faf1 into master Aug 16, 2023
20 checks passed
@pierredup pierredup deleted the builder-factory branch August 16, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gateways not registered in Symfony 6
2 participants