Missing return value in Magento\CmsUrlRewrite\Plugin\Cms\Model\Store\View::afterSave can cause saving a storeview to crash

hostep commented 4 years ago

Preconditions (*)

Tested on a vanilla Magento 2.3.5 and 2.4-develop (commit 735579d8aa14f7275d92818848f4a405191e7cb2) instance

Steps to reproduce (*)

Setup a vanilla Magento installation
In the backend, go to Stores > All Stores
Open the 'Default Store View'
Save the storeview, no need to change anything, notice that this works fine

Now simulate we installed a bunch of custom modules which changes the sort order of certain Magento modules in the app/etc/config.php file, so manually change that file so Magento_CatalogUrlRewrite comes later then Magento_CmsUrlRewrite:

--- app/etc/config.php  2020-07-08 11:42:38.000000000 +0200
+++ app/etc/config2.php 2020-07-08 11:42:35.000000000 +0200
@@ -30,7 +30,6 @@
     'Magento_Payment' => 1,
     'Magento_CatalogRuleGraphQl' => 1,
     'Magento_CatalogRule' => 1,
-        'Magento_CatalogUrlRewrite' => 1,
     'Magento_StoreGraphQl' => 1,
     'Magento_Widget' => 1,
     'Magento_Quote' => 1,
@@ -39,6 +38,7 @@
     'Magento_CmsGraphQl' => 1,
     'Magento_EavGraphQl' => 1,
     'Magento_CmsUrlRewrite' => 1,
+        'Magento_CatalogUrlRewrite' => 1,
     'Magento_CmsUrlRewriteGraphQl' => 1,
     'Magento_User' => 1,
     'Magento_Msrp' => 1,

Flush the caches: bin/magento cache:flush (do not run bin/magento setup:upgrade because it will revert the changes you just did)
In the backend, go to Stores > All Stores and open the 'Default Store View' again

Save the storeview again and notice it throws the following error:

TypeError: Argument 2 passed to Magento\CatalogUrlRewrite\Model\Category\Plugin\Store\View::afterSave() must be an instance of Magento\Store\Model\ResourceModel\Store, null given, called in lib/internal/Magento/Framework/Interception/Interceptor.php on line 146 and defined in app/code/Magento/CatalogUrlRewrite/Model/Category/Plugin/Store/View.php:103 Stack trace:
#0 lib/internal/Magento/Framework/Interception/Interceptor.php(146): Magento\CatalogUrlRewrite\Model\Category\Plugin\Store\View->afterSave(Object(Magento\Store\Model\ResourceModel\Store\Interceptor), NULL, Object(Magento\Store\Model\Store\Interceptor))
#1 lib/internal/Magento/Framework/Interception/Interceptor.php(153): Magento\Store\Model\ResourceModel\Store\Interceptor->Magento\Framework\Interception\{closure}(Object(Magento\Store\Model\Store\Interceptor))
#2 generated/code/Magento/Store/Model/ResourceModel/Store/Interceptor.php(86): Magento\Store\Model\ResourceModel\Store\Interceptor->___callPlugins('save', Array, Array)
#3 lib/internal/Magento/Framework/Model/AbstractModel.php(654): Magento\Store\Model\ResourceModel\Store\Interceptor->save(Object(Magento\Store\Model\Store\Interceptor))
#4 lib/internal/Magento/Framework/Interception/Interceptor.php(58): Magento\Framework\Model\AbstractModel->save()
#5 lib/internal/Magento/Framework/Interception/Interceptor.php(138): Magento\Store\Model\Store\Interceptor->___callParent('save', Array)
...

Expected result (*)

Storeview gets saved without errors
All after plugins should return a value, in this case Magento\CmsUrlRewrite\Plugin\Cms\Model\Store\View::aftersSave is missing one

Actual result (*)

Saving a storeview results in an error
The after plugin is missing a return value

Discussion

Error was introduced in Magento 2.3.4 by MC-18561

Following patch seems to resolve the issue:

--- vendor/magento/module-cms-url-rewrite/Plugin/Cms/Model/Store/View.php   2020-01-10 06:20:40.000000000 +0100
+++ /vendor/magento/module-cms-url-rewrite/Plugin/Cms/Model/Store/View.php   2020-07-08 11:25:17.000000000 +0200
@@ -67,16 +67,18 @@
      * @param ResourceStore $object
      * @param ResourceStore $result
      * @param ResourceStore $store
-     * @return void
+     * @return ResourceStore
      * @SuppressWarnings(PHPMD.UnusedFormalParameter)
      */
-    public function afterSave(ResourceStore $object, ResourceStore $result, AbstractModel $store): void
+    public function afterSave(ResourceStore $object, ResourceStore $result, AbstractModel $store): ResourceStore
     {
         if ($store->isObjectNew() || $store->dataHasChangedFor('group_id')) {
             $this->urlPersist->replace(
                 $this->generateCmsPagesUrls((int)$store->getId())
             );
         }
+
+        return $result;
     }

     /**

Please provide Severity assessment for the Issue as Reporter. This information will help during Confirmation and Issue triage processes.

[x] Severity: S0 - Affects critical data or functionality and leaves users without workaround.
[ ] Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
[ ] Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
[ ] Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
[ ] Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.

(Not sure if S0 is correct here, creating a storeview feels like critical functionality, but I'm fine with S2 as well ...)

m2-assistant[bot] commented 4 years ago

Hi @hostep. 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.

Please, add a comment to assign the issue: @magento I am working on this

Join Magento Community Engineering Slack and ask your questions in #github channel.

magento-engcom-team commented 4 years ago

:white_check_mark: Confirmed by @engcom-Alfa Thank you for verifying the issue. Based on the provided information internal tickets MC-36312 were created

Issue Available: @engcom-Alfa, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

slavvka commented 4 years ago

Hi @hostep. Thank you for your report. The issue has been fixed in magento/magento2#29035 by @hostep in 2.4-develop branch Related commit(s):

The fix will be available with the upcoming 2.4.1 release.

hostep commented 4 years ago

Thanks @slavvka! Can you confirm that this bugfix will be backported to 2.3.x? Since the bug was introduced in 2.3.4. Thanks!

mad-develop commented 3 years ago

@slavvka is a backport coming? Issue is still present in 2.3.7

hostep commented 3 years ago

@mad-develop: nope, Magento gave up on fixing "small bugs" for the 2.3.x release line when 2.4.0 became available if I remember correctly

Also, all following 2.3.7+ releases will only be security fixes (and maybe super high prio bug fixes?) as far as I know.

But to fix this problem on your 2.3.x installation, you can take this patch and try to apply it via https://github.com/cweagans/composer-patches or https://github.com/vaimo/composer-patches/ or some other way, that's how we deal with these things 🙂

magento / magento2