-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
#37509 | Verification added to redis preloading to prevent array_comb… #37510
#37509 | Verification added to redis preloading to prevent array_comb… #37510
Conversation
…ay_combine receiving false as second argument
Hi @LeanderFS. Thank you for your contribution! Add the comment under your pull request to deploy test or vanilla Magento instance:
❗ Automated tests can be triggered manually with an appropriate comment:
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @LeanderFS,
Please look into my review comments. Also can you please cover this change with some automated tests like unit tests by mocking the object?
Thanks
@@ -51,7 +51,10 @@ public function load($id, $doNotTestCacheValidity = false) | |||
$redis->hGet(self::PREFIX_KEY . $key, self::FIELD_DATA); | |||
} | |||
|
|||
$this->preloadedData = array_filter(array_combine($this->preloadKeys, $redis->exec())); | |||
$redisResponse = $redis->exec(); | |||
$this->preloadedData = $redisResponse ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you check here if the redisResponse
is an array, then only do the further action otherwise set an empty array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$redis->exec()
return array so not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the method signature for exec()
in Credis_Client
class tells us that it returns array
only but internally as per the documentation of credis, for better performance wraps the phpredis library when available. but when checking the implementation in phpredis, exec() can return Redis|array|false also. Thats why I asked to add a check here if the return value is array then only move further.
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
@magento run Functional Tests CE, Functional Tests EE |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
✔️ QA Passed Preconditions
Manual Testing Scenario
Before: ✖️ Tested all the manual scenarios, no impact on regression testing. Failed tests seem flaky to me hence moving this PR to merge in progress. Thanks |
@magento run Functional Tests EE, Functional Tests CE |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
@magento run Functional Tests CE |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
e2a7b53
into
magento:2.4-develop
…ine receiving false as second argument
Description (*)
This change will ensure the preloading code will continue to run even when
exec()
returnsfalse
.Fixed Issues (if relevant)
Manual testing scenarios (*)
See #37509
Contribution checklist (*)