Update FlushCacheByTags to use after plugins instead of around plugins #27077

navarr · 2020-02-28T19:42:37Z

Description (*)

This PR contains a few small quality of life improvements to the FlushCacheByTags class:

Conversion of around plugins to after plugins.
These original plugins were created when after plugins did not contain the arguments to the original call. Now that they do, we can move to after plugins and not affect the call stack
Micro-optimization in cleanCacheByTags.
Calling array_unique in a loop, for the same variable isn't great. This change now caches it in a local variable.
Updating doc-blocks.
Newer documentation standards request the exclusion of parameters and return types that are duplicative in nature. I've also updated the small descriptions to read better

Manual testing scenarios

Perform step debugging in Plugin before/after changes and compare that tags are the same during the following scenarios:
- Product Creation
- Production Deletion

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)

Resolved issues:

resolves [Issue] Update FlushCacheByTags to use after plugins instead of around plugins #29558: Update FlushCacheByTags to use after plugins instead of around plugins

m2-assistant · 2020-02-28T19:42:39Z

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

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

navarr · 2020-02-28T20:35:20Z

Failed test: testCodeStyle is a false-positive. Code style is correct according to https://devdocs.magento.com/guides/v2.3/coding-standards/docblock-standard-general.html#functions-methods

Failing unit test is acknowledged and fix will be pushed along with any other failures once automated testing is complete.

lbajsarowicz · 2020-02-29T11:56:26Z

@magento run Functional Tests B2B

@navarr Unfortunately you need to provide DocBlock. Add them for now, and in the meantime I'm going to ask Lena about the Static Tests.

@lenaorobei I thought this changed some time ago and if there are strict types - there's no need of adding doc blocks?

lbajsarowicz

Unfortunately, you need to add the DocBlocks.

navarr · 2020-02-29T21:57:00Z

@lbajsarowicz Is that because the rule is "methods must have parameters" or b/c the rule is "tests can't fail"

If its the latter, we can fix the static test thats improperly failing. If its the former, its easy enough to do.

lbajsarowicz · 2020-03-02T04:39:41Z

Moved to manual approval, as the failing test is false negative.

drew7721 · 2020-03-03T16:26:55Z

@lbajsarowicz This is not ok to merge. It's fine to change the aroundSave in to an afterSave but not the delete. The delete makes operations before and after the proceed statement. So unless that is tested and 100% sure it will not break anything nor will it leave useless data in the Redis cache then this can NOT be merged in. Also, why is this removing docs from the constructor?

drew7721 · 2020-03-03T16:29:39Z

@lbajsarowicz Also, the clean method in the Magento\Framework\Cache\FrontendInterface is expecting an array type. What will happen when null is passed?? Seriously?!

This will introduce more bugs than it fixes. I agree it should be an afterSave but the rest of the changes are 👎 .

drew7721 · 2020-03-03T16:30:59Z

lib/internal/Magento/Framework/App/Cache/FlushCacheByTags.php

        foreach ($this->cacheList as $cacheType) {
            if ($this->cacheState->isEnabled($cacheType)) {
                $this->cachePool->get($cacheType)->clean(
                    \Zend_Cache::CLEANING_MODE_MATCHING_ANY_TAG,
-                    \array_unique($tags)
+                    $uniqueTags = $uniqueTags ?? \array_unique($tags)


Why? Is this really needed?

Previously the code was performing array_unique in a loop (slow!) Caching the value locally makes sense.

I agree, so let's do it once before and pass the variable to the for each. Write easy to read code and easy to debug code. Debugging that will be a pain in the future.

drew7721 · 2020-03-03T16:31:33Z

lib/internal/Magento/Framework/App/Cache/FlushCacheByTags.php

@@ -37,12 +37,6 @@ class FlushCacheByTags
     */
    private $tagResolver;

-    /**


Please don't remove code blocks. If you want add the long class declarations back instead.

Re-reviewing this, this was an erroneous removal as the type for $cacheList disappears. I'll return this block.

drew7721 · 2020-03-03T16:34:19Z

lib/internal/Magento/Framework/App/Cache/FlushCacheByTags.php

        $tags = $this->tagResolver->getTags($object);
-        $result = $proceed($object);


The tags are retrieved before deleting the Resource, not after. But we need to Purge the cache only after the delete. If the delete fails, it should rollback the transaction and not commit, hence not purge the cache. This needs to stay an around.

A rollback will throw an Exception, right? In which case the line after $proceed is not called. This same scenario occurs with the after; an exception is thrown and the after is not called.

I failed to find any instance in which the tags changed after an object was deleted. This would be the only reason to call getTags before deletion.

I'm not sure if the entityid to crate the tag will still be available on the object once it's deleted. Need to debug and test that to make sure the tag will be created the same way once the object is deleted. The object might be a product, saleable item, category, cms page, etc... This needs to account for all these types.

lib/internal/Magento/Framework/App/Cache/FlushCacheByTags.php

ghost · 2020-03-03T16:51:44Z

@drew7721 unfortunately, only members of the maintainers team are allowed to add progress related labels to the pull request

ghost · 2020-03-03T16:51:44Z

@drew7721 unfortunately, only members of the maintainers team are allowed to remove progress related labels to the pull request

navarr · 2020-04-20T15:54:13Z

@magento rerun

sidolov · 2020-08-26T14:29:11Z

@navarr looks like something went wrong, I have closed and re-opened PR and check is passed now.

sidolov · 2020-08-26T14:37:11Z

@magento run all tests

magento-engcom-team · 2020-08-26T16:36:42Z

Hi @sidolov, thank you for the review.
ENGCOM-8079 has been created to process this Pull Request
✳️ @sidolov, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

lbajsarowicz

✔️ All green from my side.

magento-engcom-team · 2020-09-19T12:35:48Z

Hi @lbajsarowicz, thank you for the review.
ENGCOM-8079 has been created to process this Pull Request

engcom-Bravo · 2020-09-30T11:37:14Z

✔️ QA Passed

Manual testing scenario

Configure xDebug
Open the file lib/internal/Magento/Framework/App/Cache/FlushCacheByTags.php
Put debug on the following lines and Start listening...

73 $this->cleanCacheByTags($tags);
93 $this->cleanCacheByTags($tags);

Go to Magento admin and Add a new product. Click Save
When xDebug stops, check the $tags array
Go to the next step, and verify that the Product has been successfully saved
Delete the previously created product from Magento admin
When xDebug stops, check the $tags array
Assert that the $tags after saving are the same as tags after deleting

AR: The $tags array is the same

…around plugins #27077

m2-assistant · 2020-09-30T19:39:17Z

Hi @navarr, 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.

ghost added the Progress: pending review label Feb 28, 2020

magento-engcom-team added Component: App Release Line: 2.4 Partner: Mediotype partners-contribution Pull Request is created by Magento Partner tech-partners-contribution Technology Partner: Vertex labels Feb 28, 2020

lbajsarowicz suggested changes Feb 29, 2020

View reviewed changes

ghost assigned lbajsarowicz Feb 29, 2020

ghost added Progress: needs update and removed Progress: pending review labels Feb 29, 2020

ghost added Progress: to approve and removed Progress: needs update labels Mar 2, 2020

drew7721 reviewed Mar 3, 2020

View reviewed changes

drew7721 suggested changes Mar 3, 2020

View reviewed changes

drew7721 added Progress: needs update and removed Progress: to approve labels Mar 3, 2020

ghost added Progress: to approve and removed Progress: needs update labels Mar 3, 2020

Merge branch '2.4-develop' into navarr/update-flush-cache-by-tags

6b5a983

magento deleted a comment from m2-assistant bot Aug 26, 2020

sidolov approved these changes Aug 26, 2020

View reviewed changes

ghost assigned sidolov Aug 26, 2020

ghost added Progress: ready for testing and removed Progress: pending review labels Aug 26, 2020

magento-engcom-team added the Partner: Blue Acorn iCi label Aug 26, 2020

ihor-sviziev added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Aug 27, 2020

navarr requested a review from lbajsarowicz August 27, 2020 14:57

lbajsarowicz approved these changes Sep 19, 2020

View reviewed changes

ghost assigned lbajsarowicz Sep 19, 2020

engcom-Bravo self-assigned this Sep 29, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Sep 29, 2020

engcom-Hotel self-assigned this Sep 30, 2020

engcom-Bravo added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Sep 30, 2020

ghost added Progress: extended testing Progress: accept and removed Progress: testing in progress Progress: extended testing labels Sep 30, 2020

magento-engcom-team pushed a commit that referenced this pull request Sep 30, 2020

ENGCOM-8079: Update FlushCacheByTags to use after plugins instead of …

49a9b84

…around plugins #27077

magento-engcom-team merged commit 4387884 into magento:2.4-develop Sep 30, 2020

navarr deleted the navarr/update-flush-cache-by-tags branch January 29, 2021 14:44

		$tags = $this->tagResolver->getTags($object);
		$result = $proceed($object);

Update FlushCacheByTags to use after plugins instead of around plugins #27077

Update FlushCacheByTags to use after plugins instead of around plugins #27077

Uh oh!

Conversation

navarr commented Feb 28, 2020 • edited by m2-assistant bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Manual testing scenarios

Contribution checklist (*)

Resolved issues:

Uh oh!

m2-assistant bot commented Feb 28, 2020

Uh oh!

navarr commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lbajsarowicz commented Feb 29, 2020

Uh oh!

lbajsarowicz left a comment

Choose a reason for hiding this comment

Uh oh!

navarr commented Feb 29, 2020

Uh oh!

lbajsarowicz commented Mar 2, 2020

Uh oh!

drew7721 commented Mar 3, 2020

Uh oh!

drew7721 commented Mar 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost commented Mar 3, 2020

Uh oh!

ghost commented Mar 3, 2020

Uh oh!

navarr commented Apr 20, 2020

Uh oh!

sidolov commented Aug 26, 2020

Uh oh!

sidolov commented Aug 26, 2020

Uh oh!

magento-engcom-team commented Aug 26, 2020

Uh oh!

lbajsarowicz left a comment

Choose a reason for hiding this comment

Uh oh!

magento-engcom-team commented Sep 19, 2020

Uh oh!

engcom-Bravo commented Sep 30, 2020

Manual testing scenario

Uh oh!

m2-assistant bot commented Sep 30, 2020

Uh oh!

Uh oh!

navarr commented Feb 28, 2020 •

edited by m2-assistant bot

Loading

navarr commented Feb 28, 2020 •

edited

Loading