buckaroo-it / Magento2

Repository containing the Magento2 plugin by Buckaroo
MIT License
28 stars 35 forks source link

[BP-1218] Version 1.39: Cannot instantiate interface Magento\Framework\Filesystem\DriverInterface #446

Closed markvds closed 2 years ago

markvds commented 2 years ago

After upgrading to the latest version (1.39), Magento complains about not being able to instantiate the interface \Magento\Framework\Filesystem\DriverInterface. This interface has been injected into the constructor of \Buckaroo\Magento2\Model\Push and it has not been configured in di.xml, nor has Magento set any preference for this class anywhere in the core.

Magento version: 2.3.7-p2, but also in 2.4 I cannot find a preference set for DriverInterface

Buckaroo-Rens commented 2 years ago

@markvds Thanks, we indeed figured out that several installations have this issue. Were we are unable to reproduce it. Please check #441 for a possible solution. Can you let us know if this works for you?

markvds commented 2 years ago

I can confirm that this fixes the issue. Thanks for the quick reply!

I think it's good to leave this issue open until a new release has been done so other people can easily find the solution.

Buckaroo-Rens commented 2 years ago

Will do ;) Thanks again for your imput and confirmation.

yaroslav-qlicks commented 2 years ago

I can confirm that this fixes the issue. Thanks for the quick reply!

I think it's good to leave this issue open until a new release has been done so other people can easily find the solution.

why just not release it asap? just create merge PR and assign new tag 1.39.1 and problem solved so many merchants should spend time on find solution, patch, put downtime on webshop and upgrade magento

Buckaroo-Rens commented 2 years ago

@yaroslav-qlicks Thanks for your response. To give you a little insight in this. There are multiple reason why we waited with this;

  1. The issue does only apply to a few merchants for some reason. Less then 5%.
  2. Updating means that we force some agencies to roll out a update due to SLA contract with merchants, something that we don't want to force so close after a release unless it really necessary.
  3. Downtime seems for me (but please correct me if i'm wrong) very unlikely, i hope non of our partners or other agencies put something live without testing it.
  4. Last but not least, it was the end of the year, lots of holidays, release freezes etc

We are currently rounding up and planning a release in the coming period (sprint).

yaroslav-qlicks commented 2 years ago

@yaroslav-qlicks Thanks for your response. To give you a little insight in this. There are multiple reason why we waited with this;

  1. The issue does only apply to a few merchants for some reason. Less then 5%.
  2. Updating means that we force some agencies to roll out a update due to SLA contract with merchants, something that we don't want to force so close after a release unless it really necessary.
  3. Downtime seems for me (but please correct me if i'm wrong) very unlikely, i hope non of our partners or other agencies put something live without testing it.
  4. Last but not least, it was the end of the year, lots of holidays, release freezes etc

We are currently rounding up and planning a release in the coming period (sprint).

thanks for your reply origin problem is: I do not know why but dotmailer/dotmailer-magento2-extension add preference for

<preference for="Magento\Framework\Filesystem\DriverInterface" type="Magento\Framework\Filesystem\Driver\File" />

and some merchants which are not used dotmailer ignore this extension via composer, so extension is not present and preference is gone

IMHO, PR https://github.com/buckaroo-it/Magento2/pull/441 is not correct and it should be fixed like this:


<type name="Buckaroo\Magento2\Model\Push">
  <arguments>
      <argument name="fileSystemDriver" xsi:type="object">Magento\Framework\Filesystem\Driver\File</argument>
  </arguments>
</type>`
Buckaroo-Rens commented 2 years ago

@yaroslav-qlicks Thanks for your input. Altho i don't agree with our fix being 'not correct' (it's just another approach') we will discuss your approach with our team and most likely adopt it after it's fully tested. Thanks again and you will hear from us this week.

yaroslav-qlicks commented 2 years ago

Altho i don't agree with our fix being 'not correct'

You are right, I just use too straight words with 'not correct'. This PR https://github.com/buckaroo-it/Magento2/pull/441 works of course when we inject Magento\Framework\Filesystem\Driver\File we broke Dependency inversion principle but with injection di.xml via arguments we keep it ;)

markvds commented 2 years ago

And then still ... If you want to be 100% clean, use \Magento\Framework\Lock\LockManagerInterface for locking purposes instead of writing your own locking mechanism. It also supports different backends for load balancing purposes.

Buckaroo-Rens commented 2 years ago

@yaroslav-qlicks Thanks again :) And @markvds Of course, and i fully agree with you on that point.

As said, we will discuss this internally and come back on this point this week.

Buckaroo-Rens commented 2 years ago

@yaroslav-qlicks As you can see we have implemented your approach. Again, thanks for your input!

Buckaroo-Rens commented 2 years ago

Issue is released in 1.40.0 thanks again for all the imput @markvds & @yaroslav-qlicks