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

Cache type without "instance" causes exception when disabling the module through "Cache Management" in the backend #26224

Closed
Serializator opened this issue Jan 1, 2020 · 9 comments · Fixed by #27307
Assignees
Labels
Fixed in 2.4.x The issue has been fixed in 2.4-develop branch Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Reported on 2.3.3 Indicates original Magento version for the Issue report.

Comments

@Serializator
Copy link

Serializator commented Jan 1, 2020

Preconditions (*)

  1. Magento 2.3.3 CE

Steps to reproduce (*)

  1. Create a cache type using "cache.xml" without the "instance" argument
<config .../>
    <type name="exceptional_cache_type">
        <label>Exceptional Cache Type</label>
        <description>An exceptional cache type using a quantum computer</description>
    </type>
</config>
  1. Go to "System > Cache Management" in the backend
  2. Check the checkbox of the cache type "Exceptional Cache Type"
  3. Select "Disable" and click "Submit"

Expected result (*)

I'd expect the cache type to be successfully disabled.

According to the XSD ("lib/internal/Magento/Framework/Cache/etc/cache.xsd") the "instance" argument is optional and not required.

Actual result (*)

It throws an exception because it's trying to invoke ->clean(...) on a null variable.

Fatal error: Uncaught Error: Call to a member function clean() on null in /lib/internal/Magento/Framework/App/Cache/TypeList.php:194 Stack trace:
#0 /app/code/Magento/Backend/Controller/Adminhtml/Cache/MassDisable.php(62): Magento\Framework\App\Cache\TypeList->cleanType('invalid_cache_t...')
#1 /app/code/Magento/Backend/Controller/Adminhtml/Cache/MassDisable.php(41): Magento\Backend\Controller\Adminhtml\Cache\MassDisable->disableCache()
#2 /generated/code/Magento/Backend/Controller/Adminhtml/Cache/MassDisable/Interceptor.php(24): Magento\Backend\Controller\Adminhtml\Cache\MassDisable->execute()
#3 /lib/internal/Magento/Framework/App/Action/Action.php(107): Magento\Backend\Controller\Adminhtml\Cache\MassDisable\Interceptor->execute()
@m2-assistant
Copy link

m2-assistant bot commented Jan 1, 2020

Hi @Serializator. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.4-develop instance - upcoming 2.4.x release

For more details, please, review the Magento Contributor Assistant documentation.

@Serializator do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?

  • yes
  • no

@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Jan 1, 2020
@eduard13 eduard13 self-assigned this Jan 2, 2020
@m2-assistant
Copy link

m2-assistant bot commented Jan 2, 2020

Hi @eduard13. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: 👇

  • 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).

    DetailsIf the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

  • 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.

  • 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • 4. Verify that the issue is reproducible on 2.4-develop branch

    Details- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!


@eduard13
Copy link
Contributor

eduard13 commented Jan 2, 2020

Hi @Serializator, I was able to reproduce it, and working on a fix for it. Please note that I've updated the description a bit, by adjusting the path for the XSD file.
Thanks.

@rogyar
Copy link
Contributor

rogyar commented Jan 2, 2020

Hi folks. I can confirm that this issue appears. But is there a use case for a cache definition without an implementation? I mean, maybe it's better to throw an exception to notify the developer that the parameter is missing than silently accept the "ghost" cache type?

@Serializator
Copy link
Author

Hi @rogyar, I think there are two possible solutions to the problem and it depends on what the leading factor is.

If you take the XSD as "source of truth" (or whatever you wanna call it, hahah) it shouldn't throw the exception. If you take the implementation as the leading factor the XSD should be updated and make the "instance" parameter required.

I talked a little with @eduard13 in the Magento Community Engineering Slack about why Magento\Framework\App\Cache\TypeList is using a different factory than Magento\Framework\App\Cache\Frontend\Pool.

Magento\Framework\App\Cache\TypeList uses Magento\Framework\App\Cache\InstanceFactory, which basically instantiates the given class using the object manager and validates that its actually of type Magento\Framework\Cache\FrontendInterface.

Magento\Framework\App\Cache\Frontend\Pool uses Magento\Framework\App\Cache\Frontend\Factory. Note that I'm making the assumption that both factories have the same purpose because they both return classes of type Magento\Framework\App\Cache\FrontendInterface. I'm also assuming that $frontend['type'] on line 150 in Magento\Framework\App\Cache\Frontend\Pool is equivalent to the "instance" parameter.

A lot of assumptions, I know.

If $frontend['type'] doesn't exist it uses Magento\Framework\Cache\Core as the type, negating the possible null exception that we have when using Magento\Framework\App\Cache\InstanceFatory.

@rogyar
Copy link
Contributor

rogyar commented Jan 6, 2020

Hi @Serializator. Thanks very interesting observation and absolutely makes sense for me. Thank you for the detailed description.

I believe we need to figure out why the TypeList uses a different factory and what is the best way to proceed in case if the frontend type is null. I will raise this question during an internal discussion.

@Serializator
Copy link
Author

Hi @rogyar, any progress in regards to the internal discussion or not yet? Just checking out of curiosity 😉

@rogyar
Copy link
Contributor

rogyar commented Feb 4, 2020

Still in progress. I will get back here as soon as I have green/red light on how to proceed with this one.

Thank you

@ghost ghost assigned andrewbess and unassigned eduard13 Feb 28, 2020
@ghost ghost assigned andrewbess and unassigned andrewbess Mar 17, 2020
@ghost ghost added the Progress: done label Mar 25, 2020
@magento-engcom-team magento-engcom-team added the Fixed in 2.4.x The issue has been fixed in 2.4-develop branch label May 1, 2020
@magento-engcom-team
Copy link
Contributor

Hi @Serializator. Thank you for your report.
The issue has been fixed in #27307 by @andrewbess in 2.4-develop branch
Related commit(s):

The fix will be available with the upcoming 2.4.0 release.

@magento-engcom-team magento-engcom-team added the Reported on 2.3.3 Indicates original Magento version for the Issue report. label Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.4.x The issue has been fixed in 2.4-develop branch Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Reported on 2.3.3 Indicates original Magento version for the Issue report.
Projects
None yet
5 participants