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

Removed "MAGE" cache tag #3246

Merged
merged 3 commits into from
May 13, 2023
Merged

Removed "MAGE" cache tag #3246

merged 3 commits into from
May 13, 2023

Conversation

fballiano
Copy link
Contributor

This PR implements the suggestions discussed in #1226 and fixes it.

The MAGE cache tag is only used in order to be able to flush all cache without actually flushing the whole cache storage and since sharing the cache storage with another project/software/whatever would be a very bad practice, we can think about safely remove it.

Removing it will allow for less cache storage space usage and less data to process.

Questions that need to be answered 1

protected function _generateCache()
{
/** @var Mage_Admin_Model_Resource_Variable_Collection $collection */
$collection = Mage::getResourceModel('admin/variable_collection');
$collection->addFieldToFilter('is_allowed', ['eq' => 1]);
$data = $collection->getColumnValues('variable_name');
$data = array_flip($data);
Mage::app()->saveCache(
Mage::helper('core')->jsonEncode($data),
self::CACHE_ID,
[Mage_Core_Model_App::CACHE_TAG]
);
}

This method only uses Mage_Core_Model_App::CACHE_TAG as cache tag, what are we doing with that method?

Questions that need to be answered 2

protected function _generateCache()
{
/** @var Mage_Admin_Model_Resource_Block_Collection $collection */
$collection = Mage::getResourceModel('admin/block_collection');
$collection->addFieldToFilter('is_allowed', ['eq' => 1]);
$disallowedBlockNames = $this->getDisallowedBlockNames();
if (is_array($disallowedBlockNames) && count($disallowedBlockNames) > 0) {
$collection->addFieldToFilter('block_name', ['nin' => $disallowedBlockNames]);
}
$data = $collection->getColumnValues('block_name');
$data = array_flip($data);
Mage::app()->saveCache(
Mage::helper('core')->jsonEncode($data),
self::CACHE_ID,
[Mage_Core_Model_App::CACHE_TAG]
);
}

same thing as before

Questions that need to be answered 3

public function clean($tags = [])
{
$mode = Zend_Cache::CLEANING_MODE_MATCHING_ANY_TAG;
if (!empty($tags)) {
if (!is_array($tags)) {
$tags = [$tags];
}
$res = $this->getFrontend()->clean($mode, $this->_tags($tags));
} else {
$res = $this->getFrontend()->clean($mode, [Mage_Core_Model_App::CACHE_TAG]);
$res = $res && $this->getFrontend()->clean($mode, [Mage_Core_Model_Config::CACHE_TAG]);
}
return $res;
}

do we just replace this method with the flush() method?

Fixed Issues

  1. Fixes Remove "MAGE" cache tag #1226

@github-actions github-actions bot added the Component: Core Relates to Mage_Core label May 11, 2023
@colinmollenhour
Copy link
Member

This method only uses Mage_Core_Model_App::CACHE_TAG as cache tag, what are we doing with that method?

Ideally they are associated with a cache type, I'd say either config (Mage_Core_Model_Config::CACHE_TAG) or collections (Mage_Core_Model_Resource_Db_Collection_Abstract::CACHE_TAG) or add a new tag or cache type.

do we just replace this method with the flush() method?

Personally I think flush() should be used since this is fast and won't leak any untagged entries, but if some are opposed to that I think looping over the types would also work:

foreach ($this->getTypes() as $type => $flag) {
    $this->cleanType($type);
}

@github-actions github-actions bot added the Component: Admin Relates to Mage_Admin label May 11, 2023
@fballiano
Copy link
Contributor Author

all done, thanks @colinmollenhour as usual!

I think this is ready for review now

@fballiano fballiano merged commit 0e8319c into OpenMage:main May 13, 2023
@fballiano fballiano deleted the magecache branch May 13, 2023 10:38
@sreichel
Copy link
Contributor

sreichel commented May 14, 2023

Merged latest into #3248 and see ...

FAILED
ERROR: Warning: Undefined variable $res  in /var/www/html/app/code/core/Mage/Core/Model/Cache.php on line 426

@fballiano
Copy link
Contributor Author

can't reproduce this problem with "main" and developer_mode on, how do you find it?

@sreichel
Copy link
Contributor

sreichel commented May 14, 2023

Not at home atm ... run clean() method from script or test my ddev command.

$res is not set ...

@fballiano
Copy link
Contributor Author

true, PR coming in 3 seconds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Admin Relates to Mage_Admin Component: Core Relates to Mage_Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove "MAGE" cache tag
5 participants