-
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
Fix type hint of customer-data updateSectionId parameters #16115
Fix type hint of customer-data updateSectionId parameters #16115
Conversation
The typehint wrongly indicates the parameter should be a number, where in fact it is a boolean. It is passed through to the server side to the controller action \Magento\Customer\Controller\Section\Load::execute() where it is cast to a boolean and passed as the $updateIds parameter to \Magento\Customer\CustomerData\SectionPool::getSectionsData() and from there as the boolean parameter $forceUpdate to the \Magento\Customer\CustomerData\Section\Identifier::initMark() method. This PR introduces no backward incompatibilities, it only fixes the type hint to make the code more readable and improve compatibility with static code analysis tools.
Hi @Vinai. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
I already signed the contributor license agreement years ago. |
@Vinai maybe CLA was changed? :) I had to re-sign few days ago as well. |
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.
getFromServer: function (sectionNames, updateSectionId) {
var parameters;
sectionNames = sectionConfig.filterClientSideSections(sectionNames);
parameters = _.isArray(sectionNames) ? {
sections: sectionNames.join(',')
} : [];
parameters['update_section_id'] = updateSectionId;
The code looks quite confusing currently. Yeah, I see that second parameter of reload()
is true
or false
but it does not really help when you read the method code.
Could you please rename those into something like
parameters['is_section_id_update_required'] = isSectionIdUpdateRequired;
(I didn't check exact semantics, naming is up to you). I believe such change will not be a BC break as well.
I renamed the variable to be more intent revealng. The |
Why is the Travis build not starting? And it’s not only this PR... |
For reference a copy of a chat on slack:
|
@Vinai no idea yet, Magento guys are aware of it. |
Hi @orlangur, thank you for the review. |
Hi @Vinai. Thank you for your contribution. Please, consider to port this solution to 2.3 release line. |
Description
The type hint wrongly indicates the parameter should be a number, where in fact it is a boolean.
It is passed through to the server side to the controller action
\Magento\Customer\Controller\Section\Load::execute()
where it is cast to a bool and passed as the
$updateIds
parameter to\Magento\Customer\CustomerData\SectionPool::getSectionsData()
,and from there as the boolean parameter
$forceUpdate to the
\Magento\Customer\CustomerData\Section\Identifier::initMark()` method.This PR introduces no backward incompatibilities, it only fixes the type
hint to make the code more readable and improve compatibility with static
code analysis tools.
Fixed Issues (if relevant)
I didn't bother first opening an issue about this small change.
Manual testing scenarios
None, since it's only a comment change.
Contribution checklist