gwharton / autocustomergroup

Magento 2 Module - Auto Assign Customer Group based on Tax Scheme validation
Other
17 stars 4 forks source link

Module doesn't work on Magento 2.3.x #7

Closed hostep closed 3 years ago

hostep commented 3 years ago

Hi again!

When trying out this module (version 0.2.2) on a Magento 2.3.7 shop and running bin/magento setup:di:compile, we get the following error:

$ bin/magento setup:di:compile
Compilation was started.
Interceptors generation... 4/8 [==============>-------------]  50% 19 secs 128.0 MiB
In Generator.php line 139:

  Class Magento\Framework\View\Helper\SecureHtmlRenderer does not exist
  Class Gw\AutoCustomerGroup\Block\Adminhtml\ValidateVATNumberButton\Interceptor generation error: The requested class did not generate properly, because the 'generated' directory permission is read-only. If --- after running the 'bin/
  magento setup:di:compile' CLI command when the 'generated' directory permission is set to write --- the requested class did not generate properly, then you must add the generated class object to the signature of the related construct
   method, only.

setup:di:compile

It looks like the SecureHtmlRenderer class doesn't exist yet in Magento 2.3.7, it was only introduced in Magento 2.4.0.

Is there a way to make this module compatible with Magento 2.3.x, or is it only meant to be used with Magento 2.4.x ?

Thanks! 🙂

gwharton commented 3 years ago

Let me know if the following commit works for you.

https://github.com/gwharton/autocustomergroup/commit/428db7b9b36bcc3b366dd4469ae38fd61710f416

I am unable to test it as my dev node is running 7.4 and I cant deploy a 2.3 test instance.

To test the change,

make sure setup:di:compile works,

and then inspect the "Vaslidate Tax Identifier" link on the admin create order page

image

and check that there is a script tag defining the variable billingAddressParameters directly above the

hostep commented 3 years ago

Nice, thanks for the quick fix!

Just tested it and it almost worked.

2 problems:

Problem 1: When trying to create an order in the backoffice of Magento, I got the following error:

Exception #0 (UnexpectedValueException): Invalid parameter configuration provided for $reader argument of Magento\Framework\Interception\PluginList\PluginList
<pre>#1 Magento\Framework\ObjectManager\Factory\AbstractFactory->getResolvedArgument() called at [vendor/magento/framework/ObjectManager/Factory/AbstractFactory.php:236]
#2 Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgumentsInRuntime() called at [vendor/magento/framework/ObjectManager/Factory/Dynamic/Developer.php:34]
#3 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->_resolveArguments() called at [vendor/magento/framework/ObjectManager/Factory/Dynamic/Developer.php:59]
#4 Magento\Framework\ObjectManager\Factory\Dynamic\Developer->create() called at [vendor/magento/framework/ObjectManager/ObjectManager.php:70]
#5 Magento\Framework\ObjectManager\ObjectManager->get() called at [vendor/magento/framework/Interception/Interceptor.php:42]
#6 Gw\AutoCustomerGroup\Block\Adminhtml\ValidateVATNumberButton\Interceptor->___init() called at [generated/code/Gw/AutoCustomerGroup/Block/Adminhtml/ValidateVATNumberButton/Interceptor.php:13]
#7 Gw\AutoCustomerGroup\Block\Adminhtml\ValidateVATNumberButton\Interceptor->__construct() called at [vendor/magento/framework/ObjectManager/Factory/AbstractFactory.php:121]
...

Apparently that's a thing that happens to other people as well and has something to do with the ObjectManager: https://stackoverflow.com/q/55145670/145829

I was able to fix it by not creating an instance of the ObjectManager via DI but using its static getInstance method (or maybe it's the get vs create method, not sure):

diff --git a/Block/Adminhtml/ValidateVATNumberButton.php b/Block/Adminhtml/ValidateVATNumberButton.php
index 54fef15..bd0c6d3 100644
--- a/Block/Adminhtml/ValidateVATNumberButton.php
+++ b/Block/Adminhtml/ValidateVATNumberButton.php
@@ -28,11 +28,6 @@ class ValidateVATNumberButton extends Element
      */
     protected $serializer;

-    /**
-     * @var ObjectManager
-     */
-    private $objectManager;
-
     /**
      * @var SecureHtmlRenderer
      */
@@ -41,19 +36,16 @@ class ValidateVATNumberButton extends Element
     /**
      * @param Context $context
      * @param Json $serializer
-     * @param ObjectManager $objectManager
      * @param array $data
      */
     public function __construct(
         Context $context,
         Json $serializer,
-        ObjectManager $objectManager,
         array $data = []
     ) {
         $this->serializer = $serializer;
-        $this->objectManager = $objectManager;
         if (class_exists(SecureHtmlRenderer::class)) {
-            $this->secureRenderer = $this->objectManager->create(SecureHtmlRenderer::class);
+            $this->secureRenderer = ObjectManager::getInstance()->get(SecureHtmlRenderer::class);
         }
         parent::__construct($context, $data);
     }

Problem 2: running setup:di:compile gave me another error on Magento 2.3.7:

Interception cache generation... 6/8 [=====================>------]  75% 54 secs 286.0 MiBErrors during compilation:
    Gw\AutoCustomerGroup\Block\Adminhtml\Rate\Form
        Extra parameters passed to parent construct: $directoryHelper. File: vendor/gwharton/autocustomergroup/Block/Adminhtml/Rate/Form.php
Total Errors Count: 1

The constructor of Magento\Tax\Block\Adminhtml\Rate\Form is different between 2.3.7 and 2.4.0

Since the DirectoryHelper argument is optional, you can remove it from your Preference like so:

diff --git a/Block/Adminhtml/Rate/Form.php b/Block/Adminhtml/Rate/Form.php
index fe10cb4..90d4ecd 100644
--- a/Block/Adminhtml/Rate/Form.php
+++ b/Block/Adminhtml/Rate/Form.php
@@ -2,8 +2,6 @@
 namespace Gw\AutoCustomerGroup\Block\Adminhtml\Rate;

 use Gw\AutoCustomerGroup\Model\TaxSchemes;
-use Magento\Directory\Helper\Data as DirectoryHelper;
-use Magento\Framework\App\ObjectManager;
 use Magento\Framework\Exception\NoSuchEntityException;
 use Magento\Tax\Controller\RegistryConstants;

@@ -26,7 +24,6 @@ class Form extends \Magento\Tax\Block\Adminhtml\Rate\Form
      * @param \Magento\Tax\Model\TaxRateCollection $taxRateCollection
      * @param \Magento\Tax\Model\Calculation\Rate\Converter $taxRateConverter
      * @param array $data
-     * @param DirectoryHelper|null $directoryHelper
      * @param TaxSchemes $taxSchemes
      * @SuppressWarnings(PHPMD.ExcessiveParameterList)
      */
@@ -42,8 +39,7 @@ class Form extends \Magento\Tax\Block\Adminhtml\Rate\Form
         \Magento\Tax\Model\TaxRateCollection $taxRateCollection,
         \Magento\Tax\Model\Calculation\Rate\Converter $taxRateConverter,
         TaxSchemes $taxSchemes,
-        array $data = [],
-        ?DirectoryHelper $directoryHelper = null
+        array $data = []
     ) {
         parent::__construct(
             $context,
@@ -56,8 +52,7 @@ class Form extends \Magento\Tax\Block\Adminhtml\Rate\Form
             $taxRateRepository,
             $taxRateCollection,
             $taxRateConverter,
-            $data,
-            $directoryHelper
+            $data
         );
         $this->taxSchemes = $taxSchemes;
     }

After these two fixes, the module seems to be in a better shape on Magento 2.3.7 and that <script> tag shows up correctly when creating an order in the backoffice. (but I didn't get the chance to do more testing so far)

gwharton commented 3 years ago

Thanks for that Pieter, Changes added in latest commit.

gwharton commented 3 years ago

I'll close this issue down now, as it looks like it is working better on 2.3.x

Cheers for the contribution.

Given how complex this is, and given how VAT specific the current Magento tax and VIV code is, I can't see Adobe magically producing anything any time soon. The next 2.4 release is scheduled after 1st July, so the recent 2.4.2-p1 was the last chance to get anything out in a release form in time, and something like this was always unlikely to get pushed out in a patch release. They really need to rip out the whole TAX subsystem and redesign it from the ground up to support many different tax schemes, each with their own complex set of rules. Given how many Magento stores there are out there, what I dont understand is why Adobe arent inundated with people shouting about this. Their responses on slack seem to suggest its not a great priority for them. perhaps under the water, the swans feet are furiously paddling and we may be surprised. :)