-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Refactor: Improve mview code readability #23240
Refactor: Improve mview code readability #23240
Conversation
Hi @lbajsarowicz. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
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.
Hi @aredridel,
Changes looks good to me. Let's wait once all tests will be executed, probably some changes still be required.
Once you finish fixing code style and other issues - please squash all your changes into single commit and force push them.
Thank you
// Remove subscriptions | ||
foreach ($this->getSubscriptions() as $subscriptionConfig) { | ||
/** @var View\SubscriptionInterface $subscription */ | ||
$subscriptionInstance = $this->subscriptionFactory->create( |
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.
Looks like it could be replaced to $subscriptionInstance = $this->initSubscriptionInstance($subscriptionConfig);
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.
There's inconsistency with 'subscription_model' and 'subscriptionModel', however I'll try to handle that.
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.
OK. According to the
magento2/lib/internal/Magento/Framework/Mview/Config/Converter.php
Lines 89 to 93 in 15e3c07
$data['subscriptions'][$name] = [ | |
'name' => $name, | |
'column' => $column, | |
'subscription_model' => $subscriptionModel | |
]; |
subscription_model
not subscriptionModel
…m), removing try ... catch blocks throwing just caught exception, make use of imports and extract method initializing Subscription, use short-return structure to reduce complexity.
933988b
to
f8078de
Compare
Hi @ihor-sviziev, thank you for the review.
|
✔️ QA Passed |
Hi @lbajsarowicz, thank you for your contribution! |
Description (*)
I found
lib/internal/Magento/Framework/Mview/View.php
code really annoying to debug / investigate, so that I changed some parts to be more readable, functional.Fixed Issues (if relevant)
Irrelevant
Manual testing scenarios (*)
On schedule
reindexContribution checklist (*)