-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Allow ignored columns for mview to be specified at the subscription level #29692
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
Allow ignored columns for mview to be specified at the subscription level #29692
Conversation
Hi @aligent-lturner. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@magento run all tests |
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.
Correct formatting to fix test & fix functional tests
@magento run all tests |
@magento run all tests |
Hi @BarnyShergold, thank you for the review.
|
@magento create issue |
Hi @gabrieldagama, thank you for the review. |
✔️ QA passed |
Note: Automation tests are passed |
Hi @BarnyShergold, thank you for the review. |
Note: Automation tests are passed |
…ubscription level #29692
Hi @aligent-lturner, thank you for your contribution! |
Guys, can you please be aligned to best practices. There is specific configuration for Mview, it is called mview.xml. Why you did this in DI.xml. For what purpose? |
Because there is existing configuration for ignoring columns in di.xml for the class in question. It makes sense to keep it together there. I agree that ideally, the configuration would be in mview.xml, but this would also require updating the associated xsd and would then also separate the configuration for globally ignored columns and those ignored for a specific view/table |
Description (*)
Current configuration options for indexer subscriptions are limited - only allowing a table to be specified, rather than specific columns that we may be interested in.
Likewise
\Magento\Framework\Mview\View\Subscription
has the argument$ignoredUpdateColumns
, which by default only includesupdated_at
. This argument is used to ignore columns for update triggers globally.This change adds another argument to
\Magento\Framework\Mview\View\Subscription
to allow for columns to be ignored for a specific view/table combination (i.e. at the subscription level).Example use-case:
You may wish to subscribe to the
salesrule
table for a custom indexer (e.g. view id of "my_custom_view"), but with current functionality, this results in a reindex occurring any time an order is placed which had the rule applied, due to thetimes_used
column being updated.With the following
di.xml
entries, we can specifically ignore thetimes_used
column for our indexer view:Manual testing scenarios (*)
di.xml
configuration for an existing view - e.g.cataloginventory_stock
:app/code/Magento/CatalogInventory/etc/di.xml
cataloginventory_stock_item.low_stock_date
does not create a record incataloginventory_stock_cl
, but does create records incatalogsearch_fulltext_cl
andcatalog_product_price_cl
Questions or comments
Not sure if there's any tests that can be updated/added for this change. Please feel free to suggest them (or even add them!)
Contribution checklist (*)
Resolved issues: