Skip to content
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

IBX-6634: Fixed relationship comparison #11

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

Steveb-p
Copy link
Contributor

@Steveb-p Steveb-p commented Nov 22, 2023

Question Answer
JIRA issue IBX-6634
Type bug
Target Ibexa version v4.6
BC breaks no

This PR fixes issues that occur when a Criterion is used that represents a relationship condition.

For example, LoggedAt criterion which involved sub-tables with "lower than" condition was overwritten with a "equal to", because comparison operator was ignored - only value was used.

Additionally, a few methods are exposed for use in descendants so that common operations when building a query can be performed without repetition.

EDIT: Additionally, this also apparently fixes an issue that happens when attempting to countBy items, while using sub-tables (pre-joined tables are missing).

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping for example @ibexa/php-dev for back-end changes and/or @ibexa/javascript-dev for
    front-end changes).

@Steveb-p Steveb-p requested a review from a team November 22, 2023 13:44
@Steveb-p Steveb-p added Bug Something isn't working Ready for review labels Nov 22, 2023
@konradoboza konradoboza requested a review from a team November 22, 2023 13:54
@bogusez
Copy link

bogusez commented Nov 22, 2023

The changes are required for example to filtering recent activity logs by object class. Now filtering works correct.

@Steveb-p
Copy link
Contributor Author

This fix has a side-effect on one of the pages in Ibexa DXP, and so needs revision.

@Steveb-p Steveb-p marked this pull request as draft November 23, 2023 08:34
@Steveb-p
Copy link
Contributor Author

2ae1a4c seems to fix the issue of additional SELECT statements being applied to the countBy operation.

@Steveb-p Steveb-p marked this pull request as ready for review November 23, 2023 10:08
@konradoboza konradoboza requested a review from a team November 27, 2023 08:38
$qb->select($this->connection->getDatabasePlatform()->getCountExpression($tableAlias . '.' . $identifierColumn));
$qb->from($metadata->getTableName(), $tableAlias);
$platform = $this->connection->getDatabasePlatform();
$qb->select($platform->getCountExpression(sprintf('DISTINCT %s.%s', $tableAlias, $identifierColumn)));
Copy link
Member

Choose a reason for hiding this comment

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

There's \Doctrine\DBAL\Query\QueryBuilder::distinct but maybe not usable here given it's COUNT(DISTINCT ) not DISTINCT COUNT()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, due to the above it is not usable. I have actually checked and we have a similar code in our ibexa/core, and this syntax works for our supported databases.

Comment on lines +67 to +72
new Comparison(
'relationship_1.relationship_2.relationship_2_foo',
'IN',
'bar',
),
'relationship_2_table_name.relationship_2_foo IN :relationship_2_foo_0',
Copy link
Member

Choose a reason for hiding this comment

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

TBH I don't follow there relationship between what you declare this relationship and what you get from it (pun intended), but I think we've already discussed this ages ago, so I'll just continue scrolling :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alongosz could you rephrase? I don't quite understand what part of the above is the issue? Is it that we are declaring relationships between tables?

@alongosz alongosz requested a review from a team December 1, 2023 09:55
@tomaszszopinski tomaszszopinski self-assigned this Dec 1, 2023
Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on IbexaDXP 4.6 commerce.

@Steveb-p Steveb-p merged commit 4b2fd13 into main Dec 5, 2023
11 checks passed
@Steveb-p Steveb-p deleted the IBX-6634/relationship-comparison branch December 5, 2023 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants