Skip to content

#21853: Allow mview indexers to use different entity columns. #21857

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.

Sign up for GitHub

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

Merged
merged 25 commits into from
Jan 24, 2021
Merged

#21853: Allow mview indexers to use different entity columns. #21857

Changes from 2 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
891198c
#21853: Allow mview indexers to use different entity columns.
Mar 20, 2019
7da4940
Merge branch '2.4-develop' into 21853
slavvka Jan 24, 2020
b59fc5a
#21853: Simplify the code change.
Apr 12, 2020
4035ec0
#21853: Correct the comment.
Apr 12, 2020
320ced7
21853: Send the view object and do all processing in buildStatement.
Apr 14, 2020
0d1ac3a
Merge branch '2.4-develop' into 21853
lenaorobei Apr 14, 2020
6cbf85f
Merge branch '2.4-develop' into 21853
lenaorobei Aug 25, 2020
7c2d2a6
#21853: Fix tests.
Aug 29, 2020
8388a90
#21853: Fix tests.
Aug 31, 2020
3f67ed8
Merge branch '2.4-develop' into 21853
VladimirZaets Sep 11, 2020
873b814
Merge branch '2.4-develop' into 21853
engcom-Foxtrot Sep 25, 2020
392dbf6
Merge branch '2.4-develop-mainline' into 21853
engcom-Foxtrot Sep 25, 2020
ff56be5
magento/magento2#21853: Allow mview indexers to use different entity …
engcom-Foxtrot Sep 28, 2020
e16f2c2
Merge branch '2.4-develop-mainline' into 21853
engcom-Foxtrot Oct 1, 2020
922e029
Merge branch '2.4-develop' into 21853
engcom-Charlie Nov 12, 2020
330d595
Merge branch '2.4-develop' into 21853
engcom-Charlie Nov 19, 2020
4c1094a
extracted getting column to separate method
engcom-Charlie Nov 25, 2020
69cee97
Merge branch '2.4-develop' into 21853
engcom-Charlie Nov 25, 2020
46429a4
Merge branch '2.4-develop' into 21853
engcom-Charlie Nov 30, 2020
4ad7aae
fix unit test
engcom-Charlie Nov 30, 2020
8a9b768
define arguments & return types
engcom-Charlie Nov 30, 2020
db4cf23
Merge branch '2.4-develop' into 21853
engcom-Charlie Dec 1, 2020
094decc
Merge branch '2.4-develop' into 21853
engcom-Charlie Dec 28, 2020
910eb41
fixing conflict merge; fixed unit test
engcom-Charlie Dec 28, 2020
be3f84b
Merge branch '2.4-develop' into 21853
Jan 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions lib/internal/Magento/Framework/Mview/View/Subscription.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,19 @@ public function create()

// Add statements for linked views
foreach ($this->getLinkedViews() as $view) {
// Store current column name for reverting back later.
$originalColumnName = $this->getColumnName();
Copy link
Contributor

@lenaorobei lenaorobei Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution looks like we are trying to hack the subscription logic by changing object property and misuse the columnName .
What we actually need is subscription column name, not the changelog column name (which is all the time the same).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lenaorobei I looked at the code again and your comment and it's a bit confusing
1/ It seems we both agree we need to use the subscription column name
2/ I'm not sure why you mention not the changelog column name, we need two column names - changelog column is for the changelog table and that's not going to change (even you mentioned same). What we need is to change how we read the subscription column name

My understanding, Subscription is created once for all the subscribers together and whichever subscription we modify last - columnName is used from that and re-used for all the triggers.

I've tried to simplify the code to make it look not (or less) hacky but to me it seems changes are required in the same direction as I've taken or a major rework would be required in how the subscriptions are created (refactor how the statement is built for linked views)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, we need both changelog and subscription column names, but $this->getColumnName() is changelog column, not the subscription one.
I recommended to move the logic of getting its name to the buildStatement method in order to cover both cases - create and delete.

Copy link
Author

@nikunjkotecha nikunjkotecha Apr 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have any object / data available as of now in buildStatement to get the column name of the linked view for which buildStatement is called.

We can pass full view object or the way I've done to pass the subscription name, please advice

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we can get view from the changelog.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not at-least as per the interface

https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/Mview/View/ChangelogInterface.php

I may be wrong, please guide if I'm missing something here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.
Let's pass the view object then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


/** @var \Magento\Framework\Mview\ViewInterface $view */
// Use the column name from specific subscription instead of
// use from the one which is currently updated.
$subscriptions = $view->getSubscriptions();
$subscription = $subscriptions[$this->getTableName()];
$this->columnName = $subscription['column'];
$trigger->addStatement($this->buildStatement($event, $view->getChangelog()));

// Revert back the column name.
$this->columnName = $originalColumnName;
}

$this->connection->dropTrigger($trigger->getName());
Expand Down Expand Up @@ -146,8 +157,19 @@ public function remove()

// Add statements for linked views
foreach ($this->getLinkedViews() as $view) {
// Store current column name for reverting back later.
$originalColumnName = $this->columnName;

/** @var \Magento\Framework\Mview\ViewInterface $view */
// Use the column name from specific subscription instead of
// use from the one which is currently updated.
$subscriptions = $view->getSubscriptions();
$subscription = $subscriptions[$this->getTableName()];
$this->columnName = $subscription['column'];
$trigger->addStatement($this->buildStatement($event, $view->getChangelog()));

// Revert back the column name.
$this->columnName = $originalColumnName;
}

$this->connection->dropTrigger($trigger->getName());
Expand Down