Fixed the ordering of method procerss, that mothod that loads all ext…

[joesenova]

…ensions into config for the Payum Bundle to function correctly.

The empty config passed for twig.env replaced the $currentConfig(already set config to only the twig config) at array_replace_recursive Thus the other extensions that needs to be added to the config gets chuckedm as they are not in the $currentConfig anymore. The method addCoreGatewayFactoryConfig changed here https://github.com/Payum/Core/commit/f444c4cf8466679d775c8e6caa73eaf161cc338d

• This changed to recursivly relae the $currentConfig and twig.enc config being a small config with nothing else in it, it just discards the $cyrrentConfig ad no other config can be added. -

This was a premature assessment on my behalf, it turned out to be a complex Twig issue, where our app has many Twig overloadds and custom extensions.

[what-the-diff[bot]]

PR Summary

• Updated Conditions in BuildConfigsPass.php The change in this file removed a specific prerequisite or condition, which was to check if a component called 'twig' exists. Following this, there was usually an action performed to add something called 'core gateway factory config.' But now, instead of that process, the code has a new condition. The new condition also checks for 'twig' but then places a different call to add 'core gateway factory config.' This has been done to improve the efficiency of the function and adapt to new requirements.

[joesenova]

@pierredup @makasim

Please check this commit and merge int a new tagged release 2.5.2

Thank you.

[pierredup]

I don't think changing the order of the calls will change anything, since the config will just be added to the builder in the same way, there is nothing that would override any other config. Let's continue the discussion in #549, and if there is indeed something that needs to be changed here, then the PR can be re-opened.

[joesenova]

Have you tested this on your side to see what it does before closing the PR?

I got it working by swooping the order.

if you have a look what happens in Payum Core with the method being called you’ll see what is going on.

I have forked the branch and will use my own branch until this issue is resolved.

[pierredup]

I have a Symfony 6 setup that works without any errors.

if you have a look what happens in Payum Core with the method being called you’ll see what is going on.

Can you elaborate on this? What is happening in Payum core? On my test setup, the twig.env config is correctly being added to the builder

coreGatewayFactoryConfig^ array:12 [
  0 => "payum.template.layout"
  1 => "payum.template.obtain_credit_card"
  2 => "payum.paths"
  3 => "payum.action.get_http_request"
  4 => "payum.action.obtain_credit_card"
  5 => "twig.env"
  6 => "payum.extension.psr_logger"
  7 => "payum.extension.log_executed_actions"
  8 => "payum.extension.profile_collector"
  9 => "payum.prepend_extensions"
  10 => "payum.extension.payum.extension.storage.app_entity_paymentdetails"
  11 => "payum.extension.payum.extension.storage.app_entity_payment"

Also the commit you have linked to, has been added in 2016, so I'm not sure if that broke something if it's been there for many years

[joesenova]

Once you have loads of custom twig extensions and overloads, it completely breaks the Payum config builder.

To move that to the last thing that gets auto laded, it just makes sure that the current bundle has all it's state loaded correctly. As Twig can be overloaded and extended in so may ways.

Without being rude, I'll just leave it here as it seems you don't want to understand the problem we are facing and the complexity of our application we have. You have the code so use it you want.

I was trying to help, as this might be an issue on other complex Symfony applications too.

Have a good evening.

[joesenova]

Left:$instance dump with twig moved to the bottom - With this all works 100% Right: @instamce dump with twig above - here no storages are being registered

[joesenova]

Same dum as above, but this show the main object the above snippet is from: Again:

• Left:$instance dump with twig moved to the bottom - With this all works 100%
• Right: @instamce dump with twig above - here no storages are being registered

[pierredup]

Without being rude, I'll just leave it here as it seems you don't want to understand the problem we are facing and the complexity of our application we have.

That is exactly what I'm trying to do, I'm trying to understand the issue and figure out the root cause. Even if you have millions of custom twig extensions and overloaded config, it should not cause other configs to be cleared, so I'm trying to figure out why this might happen

[pierredup]

I'll merge the PR and create a new release, since the change shouldn't do any harm, but I think there are other weird things that are being done in your application that overrides some config that it shouldn't which is causing this issue for you specifically

[joesenova]

I’ll send you a list of custom extensions we have running , and what each of them is doing in a nutshell.

I have gone through the entire extension loading phase over and over again. I can’t see anything that is out of place or not loading/working as expected.

we have loads of these extensions and overload on twig.

[joesenova]

@pierredup

I had a look at our custom twig Filters/Functions, we have a Filter setup to get Subscription date form the User table, which has a join to the Payum Transaction table.

This does a call on the Payum gateway->execute(GetRecurringPaymentsProfileDetails).

When I took that Filter out of the equation all started working. This method in UserController includes Payum to get the current user gateway to get the transaction date and some other details.

Are there any "rules" inplace that we need to adhere to when creating these Filters that include Payum service as part of the Twig Env?

I moved the code out of the Filter directly into our User model, all works now. i however think there might be a underlying issue. Feels like when Payum was loaded into the TwigFIlters before the package config was built, causes this issue.

Can we investigate that, and see if there is a deeper issue, or if this is a on-off edge case with our app with all the Twig Extensions and Filters?

[pierredup]

@joesenova Thanks, that's really helpful and narrows the problem down by a lot.

Are there any "rules" inplace that we need to adhere to when creating these Filters that include Payum service as part of the Twig Env?

There shouldn't be, although something weird might happen during the container compilation step. The strange thing is that this seems to only be a problem when upgrading to Symfony 6, so there might be some underlying change in Symfony's DI component that causes this break, so it might be something that we need to handle in our compiler passes

I'll investigate this issue further with the information at hand

[joesenova]

No worries @pierredup, and thank you for the reply.

If you need any other information, please let me know, I have a branch where this is in a "broken" state and will be happy to pull in changes and test.

Or even do some debugging where needed on my code base to help get this issue resolved.

[pierredup]

Okay I think I have found the root cause.

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.

There are 2 options to fix this:

• Re-order the method calls, like this PR does. This will ensure that the core config is added to the Payum builder before creating the twig service (which then also requires an instance of Payum)
  • The only problem with this, is that it fixes the problem only for Twig. If any other service requires an instance of Payum, the issue will still exist (E.G when creating a custom TokenStorage, HttpRequestVerifier, or even GatewayRegistry and injecting Payum into them
• Set 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.

@joesenova I have created a PR with the second option as a resolution (#551). If you don't mind, can you please test this fix and let me know if everything is working as expected.

[joesenova]

Thanks for the feedback @pierredup ,

this makes sense, is kind of what I assumed. With the limited knowledge of Payum it was a bit difficult to get to the 100% rot cause. I agree with the 2nd way, your PR, of fixing the problem, this solves future problems too, with other extensions and dependencies.

I will be testing this fix on my environment which s broken, and will revert back soonest.

[joesenova]

@pierredup ,

I have made this change, and still run into the exact same problem.

[pierredup]

Then there might be something else that might also cause some issues. I was able to replicate the error using a Twig extension that uses the Payum service, and the change in #551 resolved the error. Maybe there is some other service config that also needs to be adjusted

[joesenova]

Thansk @pierredup

I will go through the same process I went through the weekend, and see what I can see with regards to the fix and see where it breaks.

Error:

Token storage must be configured. -> vendor/payum/core/Payum/Core/PayumBuilder.php:380

---

WORKS::

When I change:

FROM:

<service id="payum" class="Payum\Core\Payum">
    <factory service="payum.builder" method="getPayum" />
</service>

TO:

<service id="payum" class="Payum\Core\Payum" shared="false">
    <factory service="payum.builder" method="getPayum" />
</service>

And remove this shared="false" from <service id="payum.builder" class="Payum\Core\PayumBuilder" shared="false">

[pierredup]

The problem with setting shared="false" on the payum service instead of the builder, is that you will get an instance of Payum injected into the twig extension without the storages, while other instances using Payum will have the storages correctly configured. I'm curious why the shared="false" on the payum.builder service does not work. Another option is to set shared="false" on both the payum and payum.builder instances (and set public="false" on payum.builder), but this means a full re-initialisation in every place that Payum is injected, but this will be bad for performance (E.G if you have a 100 twig extensions that rely on Payum, that means 100 initialisations on every request that uses twig, which is not ideal)

[pierredup]

@joesenova Can you try the below on the vendor/payum-bundle/Resources/config/payum.xml config

diff --git a/Resources/config/payum.xml b/Resources/config/payum.xml
index 15c1de8..43b59d2 100644
--- a/Resources/config/payum.xml
+++ b/Resources/config/payum.xml
@@ -18,7 +18,7 @@

         <defaults public="true" />

-        <service id="payum.builder" class="Payum\Core\PayumBuilder">
+        <service id="payum.builder" class="Payum\Core\PayumBuilder" public="false">
             <call method="setMainRegistry">
                 <argument type="service" id="payum.static_registry" />
             </call>
@@ -46,7 +46,7 @@
             </call>
         </service>

-        <service id="payum" class="Payum\Core\Payum">
+        <service id="payum" class="Payum\Core\Payum" lazy="true">
             <factory service="payum.builder" method="getPayum" />
         </service>

[joesenova]

@pierredup

Yes will try this tomorrow first thing in the morning.

[joesenova]

@pierredup

I can confirm this is working perfectly.

Thanks for the assist, I think this is the better way of solving this issue, as the Token Storage was the next issue I was going to run into with the swop of the config loader setting Twig to load last.

If all tests pass on this change with this PR I think we have a winner.

[pierredup]

@joesenova Release 2.5.2 is now available which includes the fix. Thanks for the report and the assistance in testing!