Flaw in AbstractConfigProvider

[robbertstevens]

Hello,

Currently I am investigating an issue we have in one of our shops. The issue is that the buckaroo fee for transactions was not displayed inclusive tax. But I believe the exact issue is not relevant because the cause is much deeper than this.

I have tracked this down to the AbstractConfigProvider::__call method. What this method does is dynamically create a string which should be a constant in the class that implements AbstractConfigProvider and then pass it into $this->getConfigFromXpath(constant($constant), $store);

This however is not possible, because of Magento creating Interceptor classes for all classes, this is to enable the plugin system.

The string that is created for example XPATH_BUCKAROOFEE_PRICE_DISPLAY_CART should be static::XPATH_BUCKAROOFEE_PRICE_DISPLAY_CART but instead is static::XPATH_INTERCEPTOR_PRICE_DISPLAY_CART

This is a fundamental problem with all config providers. My suggestion is get rid of the AbstractConfigProvider::__call all together and implement the methods defined above the implementing classes instead of relying on magic.

class BuckarooFee extends AbstractConfigProvider
{
    ...
    public function getPriceDisplayCart(?Store $store): int
    {
         return $this->getConfigFromXpath(self::XPATH_BUCKAROOFEE_PRICE_DISPLAY_CART, $store);
    }
    ...
}

I will solve my issue in a template for now.

[robbertstevens]

@Buckaroo-Rens

[vladislav-padalka-hys]

@robbertstevens thank you for your report !

But as far as we know interceptors are used not in all of cases in Magento.
They are in use only in case if plugin(s) were assigned to specific class.
ConfigProvider classes exactly that case.
You can check that logic inside Magento's framework core (for example 'getInstanceType' method)
We've also debugged "$constant" variable inside method mentioned by you (AbstractConfigProvider::__call) in many places of application and we had no cases where "INTERCEPTOR" substring was in
that debug.
So we can suspect that maybe you're using some custom plugin for our ConfigProvider classes.
Let us know if so please.
As well as please provide any other additional useful details to check your issue (e.g. : exact place where that setting is in use, debug statement, maybe access to staging where it can be reproduced, etc) , because it looks like we can not reproduce it at our side and assuming that code was unchanged within last year and didn't cause issues at many merchants.

Thanks in advance !

[robbertstevens]

We indeed have an plugin on the Buckaroo\Magento2\Model\ConfigProvider\BuckarooFee this should be possible, but because of the above it breaks.

The solution I gave is really easy to implement and it will make the code much better in general. It will make the code also much easier to extend via plugins because you do not have to do some trickery with making plugins on a magic method. And you do not have to do trickery with annotation to notify the IDE that we have indeed certain methods available.

[vladislav-padalka-hys]

it's good to know that we were right in our research according to custom plugin you have

solution you gave is easy to implement, but when it's for only one config variable
when you have tens of variables, then it will be tens of copy-paste methods also which is not good assuming DRY principle, so we will remain compact (__call) based approach

[robbertstevens]

You are not repeating anything right? All the methods you will add are different and will return different values. So you will not repeat yourself.

[vladislav-padalka-hys]

making of tens similar access methods is possible and much more trivial than existing approach , but existing approach allows us to remains code more compact and easy to change if needed

[Buckaroo-Rens]

@robbertstevens Like my colleague @vladislav-padalka-hys mentioned, we are currently not planning to change this. This piece of code is in here since the start of our plugin. We are always open to new/better changes but for now we see no direct improvement for all of our merchants by changing this.

I will put your feedback on our backlog and we will discuss this later this year.

If you need some help with your original issue 'buckaroo fee for transactions was not displayed inclusive tax' please let us know.

[markvds]

I'd like to second the issue. I wrote a plugin for the iDeal config provider to apply a custom sorting on the issuers and then discovered that the module stopped working.

I have written a patch (for which I created a pull request in the TIG era) that uses another approach: just strip off the 'Interceptor' part:

diff --git a/Model/ConfigProvider/AbstractConfigProvider.php b/Model/ConfigProvider/AbstractConfigProvider.php
index aacc0fea..463c23fd 100644
--- a/Model/ConfigProvider/AbstractConfigProvider.php
+++ b/Model/ConfigProvider/AbstractConfigProvider.php
@@ -120,6 +120,10 @@ public function __call($method, $params)
             $class = get_class($this);
             $classParts = explode('\\', $class);
             $className = end($classParts);
+            if ($className == 'Interceptor') {
+                array_pop($classParts);
+                $className = end($classParts);
+            }

             /**
              * Uppercase and append it to the XPATH prefix & child class' name

You can find the commit here: ecomni/buckaroo-magento2@7ba9c35

I'd be happy to submit a new PR if you want to.

[florinel-chis]

@markvds Thank you for your comment. Need to Look at the details from @robbertstevens which look valid to me. In the meantime feel free to submit a PR, please branch off develop - Contribution Guidelines

[florinel-chis]

@robbertstevens
FYI, In this PR I am trying to tackle the raised issue - removing the magic __call() and implementing the actual functions - #246 - feedback would be welcome.

[TerrorSquad]

I'm surprised this issue still has not been solved (1.44).
@markvds I implemented the same solution, and the issue is gone.

@serpentscode please solve this in the next bugfix release.
Also, I suggest doing additional testing with Proxy classes. I only encountered issues with classes that have plugins and, therefore, Interceptors, but it is easy to imagine that Proxy classes could trigger the same bug.

[LucianTuriacArnia]

@TerrorSquad we have refactored this issue on another branch Buckaroo v2.0 #246. We are preparing a bigger release with the refactored code, changing the main branch to address this problem is not now our priority, because it will be fixed soon in the new release.