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

Fixed EZP-21444: SQL query error in fetch('content', 'keyword') with some 'sort_by' parameters #765

Merged
merged 1 commit into from Sep 25, 2013

Conversation

2 participants
@patrickallaert
Copy link
Contributor

commented Sep 20, 2013

Extends @pedroresende's PR: #726.

Issue: https://jira.ez.no/browse/EZP-21444

Pedro's patch is correct, there is no reason to join $contentAttributeTableAlias.version with ezcontentobject_name.content_version while ezcontentobject.current_version is always around.

The possible problem mentioned by @andrerom on @pedroresende's PR with sort_by set to "name" does not exist. In that case, the ezcontentobject.name field is used.

This fix also improves the query a lot by not joining with ezcontentobject_name at all with a sort_by based on attribute (as this is the case in default's ezdemo blog template). This join can potentially be very big, depending on the number of languages available.

Tests done:
Using the fetch('content', 'keyword') with all possible sort_by parameters.

@andrerom

This comment has been minimized.

Copy link
Member

commented Sep 20, 2013

+0.5 ( I feel safer with Pedros patch, this one looks like it might introduce regressions )

@patrickallaert

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2013

@andrerom:

+0.5 ( I feel safer with Pedros patch, this one looks like it might introduce regressions )

Pedro's patch does not solve the case where "class_name" is used in sort_by, resulting in an SQL error as well.

@andrerom

This comment has been minimized.

Copy link
Member

commented Sep 20, 2013

Have you seen any use of class_name sorting?

Risk wise I'm +1 on Pedro's patch for backports (incl 5.2), so the rest should afaik only be for master.

@patrickallaert

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2013

@andrerom

Have you seen any use of class_name sorting?

Some may have used class_name instead of class_identifier just to have similar objects being grouped.

I'm ok to merge this in master and only backporting the part of Pedro.

@andrerom

This comment has been minimized.

Copy link
Member

commented Sep 24, 2013

+1 (for master)

patrickallaert added a commit that referenced this pull request Sep 25, 2013

Merge pull request #765 from patrickallaert/EZP-21444
Fixed EZP-21444: SQL query error in fetch('content',  'keyword') with some 'sort_by' parameters

@patrickallaert patrickallaert merged commit fad9377 into ezsystems:master Sep 25, 2013

1 check passed

default The Travis CI build passed
Details

@patrickallaert patrickallaert deleted the patrickallaert:EZP-21444 branch Sep 25, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.