Adds missing return value in afterSave plugin.

[hostep]

Description (*)

All after plugins should always return a value, so other plugins which follow them in the stack get the correct input parameters instead of null.
It appears that in MC-18561 this was forgotten and this can cause crashes when the sort order of default Magento modules are different then in a vanilla installation (which can happen after installing certain custom modules)

See #29034 for more information

The bug was introduced in Magento 2.3.4, so it makes sense if this PR is considered to be backported to 2.3 as well

I have no interest in adding additional automatic tests for this and wouldn't even know how to approach this, so if this is required, somebody else will need to take care of this.

Thanks!

Related Pull Requests

Fixed Issues (if relevant)

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

Manual testing scenarios (*)

See #29034

Questions or comments

Not sure if this is possible, but having a static analyser test to detect after plugins without return values could have prevented this mistake to have entered the code base

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @hostep. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

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

[hostep]

@magento run all tests

[ihor-sviziev]

Hi @hostep,
Your changes looks good. Could you cover them with unit test?

[hostep]

Hi @ihor-sviziev, please re-read my opening post, sorry I simply don't have the time for tests (if it takes me 5 to 10 times longer to write tests then to write a fix, I'm not going to do it, really sorry).

[ihor-sviziev]

@hostep got it, I just missed point related to tests.

@engcom-Charlie @engcom-Kilo @engcom-Golf could you help covering this case with unit tests?

[engcom-Charlie]

@magento run all tests

[ihor-sviziev]

✔ Approved.

Failing tests looks not related to changes form this PR.

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7814 has been created to process this Pull Request

[engcom-Alfa]

✔️ QA Passed

Manual testing scenario:

1. Setup a vanilla Magento installation
2. In the backend, go to Stores > All Stores
3. Open the 'Default Store View'
4. Save the storeview, no need to change anything, notice that this works fine
5. 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,

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

Before: ✖️ Throws the following error:

After: ✔️ Store view gets saved without errors and plugin returns a value, in this case Magento\CmsUrlRewrite\Plugin\Cms\Model\Store\View::aftersSave is missing one

[m2-assistant]

Hi @hostep, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.