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

Fix sorting issue in the SQL Manager page #10991

Merged
merged 2 commits into from Oct 18, 2018

Conversation

Projects
None yet
7 participants
@khouloudbelguith
Contributor

khouloudbelguith commented Oct 12, 2018

Questions Answers
Branch? 1.7.5.x
Description? It is impossible to sort by SQL query in the SQL Manager page
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #10990
How to test? Go to BO => Database / SQL Manager page => Create at least 2 SQL Query => After that, try to apply a sort on SQL Query with the icon

This change is Reviewable

@khouloudbelguith khouloudbelguith changed the title from Fix filtering issue in the SQL Manager page to Fix sorting issue in the SQL Manager page Oct 12, 2018

@marionf

This comment has been minimized.

Contributor

marionf commented Oct 12, 2018

@khouloudbelguith
This is not a regression of 1.7.5.0, can you do it on develop branch please ?

@khouloudbelguith khouloudbelguith changed the base branch from 1.7.5.x to develop Oct 12, 2018

@khouloudbelguith khouloudbelguith changed the base branch from develop to 1.7.5.x Oct 12, 2018

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Oct 17, 2018

Hmm, I'm not sure this fix is the best we could find.

This is not the first I see about the SQL columns in a table filtering, and instead of modifying all the occurrences which could trigger errors, I would rather make a global change.

In your case:

SELECT SQL_CALC_FOUND_ROWS  a.*  FROM `ps_request_sql` a     WHERE 1    ORDER BY sql DESC  LIMIT 0, 50;

Doesn't work. But

SELECT SQL_CALC_FOUND_ROWS  a.*  FROM `ps_request_sql` a     WHERE 1    ORDER BY a.sql DESC  LIMIT 0, 50;

does, thanks to your change, and

SELECT SQL_CALC_FOUND_ROWS  a.*  FROM `ps_request_sql` a     WHERE 1    ORDER BY `sql` DESC  LIMIT 0, 50;

works as well. It seems that applying quotes around the column names would make the SQL requests working in all case, without modifying the controller.

@marionf marionf added the QA ✔️ label Oct 17, 2018

@Quetzacoalt91

Sorry my last message was unclear but I requested changes.

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Oct 17, 2018

@Quetzacoalt91 I see your point, so I am not a pro of AdminController but it seems the change should be made here in this case:

$orderBy = bqSQL($orderBy);

        if (preg_match('/[.!]/', $orderBy)) {
            $orderBySplit = preg_split('/[.!]/', $orderBy);
            $orderBy = bqSQL($orderBySplit[0]) . '.`' . bqSQL($orderBySplit[1]) . '`';
        } elseif ($orderBy) {
            $orderBy = bqSQL($orderBy);
        }

becomes:

        if (preg_match('/[.!]/', $orderBy)) {
            $orderBySplit = preg_split('/[.!]/', $orderBy);
            $orderBy = bqSQL($orderBySplit[0]) . '.`' . bqSQL($orderBySplit[1]) . '`';
        } elseif ($orderBy) {
            $orderBy = bqSQL('`' . $orderBy . '`');
        }
@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Oct 17, 2018

Almost, by looking at the existing code, a better solution would be

            $orderBy = '`' . bqSQL($orderBy) . '`';
@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Oct 17, 2018

that's what I thought initially, but in the previous condition the quote are integrated inside the bqSQL call:

$orderBy = bqSQL($orderBySplit[0]) . '.`' . bqSQL($orderBySplit[1]) . '`';

weird?

@khouloudbelguith

This comment has been minimized.

Contributor

khouloudbelguith commented Oct 18, 2018

Hi @Quetzacoalt91, @jolelievre, thanks for your review.
@Quetzacoalt91 Done, I made new changes & it works perfectly.

'title' => $this->trans('SQL query', array(), 'Admin.Advparameters.Feature'),
'filter_key' => 'a!sql',
),
'sql' => array('title' => $this->trans('SQL query', array(), 'Admin.Advparameters.Feature')),

This comment has been minimized.

@jolelievre

jolelievre Oct 18, 2018

Contributor

I think that with the other modification in AdminController, this one is no longer useful

@marionf marionf removed the QA ✔️ label Oct 18, 2018

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Oct 18, 2018

great! thank you @khouloudbelguith ^^

@PierreRambaud PierreRambaud added this to the 1.7.5.0 milestone Oct 18, 2018

@PierreRambaud PierreRambaud merged commit 084d63d into PrestaShop:1.7.5.x Oct 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Oct 18, 2018

Thanks @khouloudbelguith

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment