Prestashop: FatalError on SQL Manager

[marionf]

Describe the bug
I have a FatalThrowableError when I try to export order sql query to sql manager

To Reproduce
Steps to reproduce the behavior:

1. Go to Orders > Orders or Orders > Credit slip or Catalog > Products or Catalog > Categories
2. Click on Export to sql manager
3. Click on Save
4. See error

Before on 1.7.4.2 I had this:

Additionnal information
PrestaShop version: develop
PHP version: N/A

[marionf]

@mickaelandrieu for info

[sarjon]

@mickaelandrieu i was able to track down why it breaks. The reason is this change: https://github.com/PrestaShop/PrestaShop/commit/d7c9264b197764e72c7900814c13364f6ad5f83f

Explanation:
Newer version of PHPSQLParser returns different result for parsed SQL than the old parser. So thats why it works on stable PrestaShop release but does not work on develop branch. i could apply some fix for that, but it wouldnt be nice. :/ what do you think?

[matks]

@sarjon Do you mean that some "classic" SQL queries of Prestashop are being flagged as "invalid" by the updated PHPSQLParser ?

[sarjon]

not really @matks , if i remember correctly, $parser result in this line https://github.com/PrestaShop/PrestaShop/blob/develop/src/Adapter/SqlManager/SqlQueryValidator.php#L52 is a bit different with new PHPSQLParser thats why it breaks in develop branch.

In addition, validateParser() returns false for a valid SQL. Here's an exact line https://github.com/PrestaShop/PrestaShop/blob/develop/src/Adapter/SqlManager/SqlQueryValidator.php#L53

wouldnt it be better to try to execute SQL and catch error instead of parsing and validating query as it is not 100% reliable?

[matks]

Issue improved by https://github.com/PrestaShop/PrestaShop/pull/10380 but root cause not fixed (yet)

[matks]

@sarjon Indeed the SQL validator has issues :/ however I am not fond of the solution "run the SQL query, catch error if there is one" it feels a bit ... raw 😅 . Like "to check if this wall is solid, let's hammer it, if it breaks it was not".

Additionnaly a nice SQL validator should not only check a query is valid but also catch bad practices.

The ideal solution is to fix/rebuild the SQL validator to have something working nice. But I dont know how long that would take 😢 I'm going to look at it

[sarjon]

i know its not the best solution to execute query, but building SQL parser that can parse any query does not sound fun at all. :/

[matks]

@sarjon PierreRambaud suggested 1st the same solution you provided: execute the query and parse sql return. However if database is MyISAM, it does not allow the use of transactions (commit/rollback) so it cannot be done for INSERT/UPDATE queries.

He suggested then we use sql "EXPLAIN" keyword :) to submit the query to mysql :) that would prevent the execution of INSERT/UPDATE queries.

@mickaelandrieu this means a change of legacy code. Would it be considered a BC break andif yes, would it be acceptable ?

[sarjon]

@matks keep in mind that only SELECT sql statements are allowed. So INSERT/UPDATE/DELETE are invalid by default.

[matks]

@sarjon You are right:

• not-SELECT queries are "unauthorized" https://github.com/PrestaShop/PrestaShop/blob/develop/classes/RequestSql.php#L60-L67
• even SQL_CALC_FOUND_ROWS that I have seen used in Prestashop BO is unauthorized https://github.com/PrestaShop/PrestaShop/blob/develop/classes/RequestSql.php#L67
• and in fact after several checks the query is performed to check if it is valid

// in validateSql()
        if (empty($this->_errors) && !Db::getInstance()->executeS($sql)) {

https://github.com/PrestaShop/PrestaShop/blob/develop/classes/RequestSql.php#L191

Sorry for wasting your time @sarjon if I had read the code sooner I would have seen your solution was already implemented.

So ... shall we just drop all the useless checks and just execute the query and parse mysql errors in the end ?

[mickaelandrieu]

Maybe we could revert the update of the library? It's sad but we could try to introduce it back during the freeze, wdyt @PierreRambaud ?

[PierreRambaud]

We need to keep the latest version of PHPSQLparser, otherwise PrestaShop will be incompatible with PHP 7.2 :/
More, this function was added after the update :/

[mickaelandrieu]

So ... shall we just drop all the useless checks and just execute the query and parse mysql errors in the end ?

I don't like it, but it sounds like the perfect solution is not possible and this workaround sounds ok to me :+1:

[matks]

The migrated SQL Manager page has been hidden for 1.7.5 and will be back only for 1.7.6.
So this issue is now for 1.7.6.

However there is a new issue for 1.7.5, most of queries obtained by the scenario of this Issue
are considered not valid by the old Legacy SQL Manager (this is because we upgraded PHPSQLParser). New issue created for 1.7.5.

[marionf]

@matks I can't reproduce anymore this one develop branch, so I close it.
Maybe we can close also #10908 ?

[matks]

@marionf It's good news although I dont know it got fixed 😅

I'm a bit worried though as someone reported the issue again this last week 😢 https://github.com/PrestaShop/PrestaShop/issues/9543

[Madalina-nadari]

Hi,
I have the same "checkedForm" error when i'm trying to save the following query, on Prestashop 1.7.4.3:
SELECT p.id_product, p.active, pl.name AS 'Name',
GROUP_CONCAT(DISTINCT(cl.name) SEPARATOR ',') AS 'Categories (x,y,z...)',
p.price AS 'Price tax excluded or Price tax included',
p.id_tax_rules_group AS 'Tax rules ID',
p.wholesale_price AS 'Wholesale price',
p.on_sale AS 'On sale (0/1)',
IF(pr.reduction_type = 'amount', pr.reduction, '') AS 'Discount amount',
IF(pr.reduction_type = 'percentage', pr.reduction, '') AS 'Discount percent',
pr.from AS 'Discount from (yyyy-mm-dd)',
pr.to AS 'Discount to (yyyy-mm-dd)',
p.reference AS 'Reference #',
p.supplier_reference AS 'Supplier reference #',
psnad.name AS 'Supplier',
pm.name AS 'Manufacturer',
p.ean13 AS 'EAN13',
p.upc AS 'UPC',
p.ecotax AS 'Ecotax',
p.width AS 'Width',
p.height AS 'Height',
p.depth AS 'Depth',
p.weight AS 'Weight',
sa.quantity as 'Quantity',
p.minimal_quantity AS 'Minimal quantity',
'both' AS 'Visibility',
p.additional_shipping_cost AS 'Additional shipping cost',
p.unity AS 'Unity',
p.unit_price_ratio AS 'Unit price',
pl.description_short AS 'Short description',
pl.description AS 'Description',
IF(t.name IS NOT NULL, GROUP_CONCAT(DISTINCT(t.name) SEPARATOR ','), '') AS 'Tags (x,y,z...)',
pl.meta_title AS 'Meta title',
pl.meta_keywords AS 'Meta keywords',
pl.meta_description AS 'Meta description',
pl.link_rewrite AS 'URL rewritten',
pl.available_now AS 'Text when in stock',
pl.available_later AS 'Text when backorder allowed',
p.available_for_order AS 'Available for order (0 = No, 1 = Yes)',
'' AS 'Product available date',
p.date_add 'Product creation date',
p.show_price AS 'Show price (0 = No, 1 = Yes)',
CONCAT('https://nadari-toys.ro',
'/img/p/',
-- now take all the digits separetly as MySQL doesn't support loopsnad in SELECT statements
-- assuming we have smaller image id than 100'000 ;)
IF(CHAR_LENGTH(pi.id_image) >= 5,
-- if we have 5 digits for the image id
CONCAT(
-- take the first digit
SUBSTRING(pi.id_image, -5, 1),
-- add a slash
'/'),
''),
-- repeat for the next digits
IF(CHAR_LENGTH(pi.id_image) >= 4, CONCAT(SUBSTRING(pi.id_image, -4, 1), '/'), ''),
IF(CHAR_LENGTH(pi.id_image) >= 3, CONCAT(SUBSTRING(pi.id_image, -3, 1), '/'), ''),
if(CHAR_LENGTH(pi.id_image) >= 2, CONCAT(SUBSTRING(pi.id_image, -2, 1), '/'), ''),
IF(CHAR_LENGTH(pi.id_image) >= 1, CONCAT(SUBSTRING(pi.id_image, -1, 1), '/'), ''),
-- add the image id
pi.id_image,
-- put the image extension
'.jpg') as image_url,
CONCAT('https://nadari-toys.ro',
'/img/p/',
-- now take all the digits separetly as MySQL doesn't support loopsnad in SELECT statements
-- assuming we have smaller image id than 100'000 ;)
IF(CHAR_LENGTH(pi2.id_image) >= 5,
-- if we have 5 digits for the image id
CONCAT(
-- take the first digit
SUBSTRING(pi2.id_image, -5, 1),
-- add a slash
'/'),
''),
-- repeat for the next digits
IF(CHAR_LENGTH(pi2.id_image) >= 4, CONCAT(SUBSTRING(pi2.id_image, -4, 1), '/'), ''),
IF(CHAR_LENGTH(pi2.id_image) >= 3, CONCAT(SUBSTRING(pi2.id_image, -3, 1), '/'), ''),
if(CHAR_LENGTH(pi2.id_image) >= 2, CONCAT(SUBSTRING(pi2.id_image, -2, 1), '/'), ''),
IF(CHAR_LENGTH(pi2.id_image) >= 1, CONCAT(SUBSTRING(pi2.id_image, -1, 1), '/'), ''),
-- add the image id
pi2.id_image,
-- put the image extension
'.jpg') as image_url2,
0 AS 'Delete existing images (0 = No, 1 = Yes)',
GROUP_CONCAT(DISTINCT(CONCAT((fl.name), ':', (fvl.value), ':0')) SEPARATOR ',') AS 'Feature (Name:Value:Position)',
p.online_only AS 'Available online only (0 = No, 1 = Yes)',
p.condition AS 'Cond',
0 AS 'Customizable (0 = No, 1 = Yes)',
0 AS 'Uploadable files (0 = No, 1 = Yes)',
0 AS 'Text fields (0 = No, 1 = Yes)',
p.out_of_stock as 'Out of stock',
'1' AS 'ID',
null AS 'Action when out of stock',
null AS 'Depends on stock',
null AS 'Warehouse'
FROM pstoysproduct p

LEFT JOIN pstoysproduct_lang pl ON(p.id_product = pl.id_product)
LEFT JOIN pstoyscategory_product cp ON(p.id_product = cp.id_product)
LEFT JOIN pstoyscategory_lang cl ON(cp.id_category = cl.id_category)

LEFT JOIN pstoysspecific_price pr ON(p.id_product = pr.id_product)
LEFT JOIN pstoysproduct_tag pt ON(p.id_product = pt.id_product)
LEFT JOIN pstoystag t ON(pt.id_tag = t.id_tag)
LEFT JOIN pstoysimage pi ON(p.id_product = pi.id_product and pi.cover = 1)
LEFT JOIN pstoysimage pi2 ON(p.id_product = pi2.id_product and pi2.position = 2)
LEFT JOIN pstoysmanufacturer pm ON(p.id_manufacturer = pm.id_manufacturer)
LEFT JOIN pstoyssupplier psnad ON(p.id_supplier = psnad.id_supplier)
LEFT JOIN pstoysconfiguration conf ON conf.name = 'pstoysSHOP_DOMAIN'
LEFT JOIN pstoysfeature_product fp ON p.id_product = fp.id_product
LEFT JOIN pstoysfeature_lang fl ON fp.id_feature = fl.id_feature
LEFT JOIN pstoysfeature_value_lang fvl ON fp.id_feature_value = fvl.id_feature_value
LEFT JOIN pstoysfeature f ON fp.id_feature = f.id_feature
LEFT JOIN pstoysfeature_value fv ON fp.id_feature_value = fv.id_feature_value
LEFT JOIN pstoysstock_available sa ON (p.id_product = sa.id_product)
WHERE pl.id_lang = 2
AND cl.id_lang = 1
GROUP BY p.id_product;

Could someone please tell me how can i fix this or if i have to upgrade to 1.7.5.0 so that i can export my products?
Thank you!

[PierreRambaud]

You need to upgrade to the latest version of PrestaShop to get the fix :)

[Madalina-nadari]

Ok. Thanks a lot!

[matks]

You need to upgrade to the latest version of PrestaShop to get the fix :)

Mmmm ... not sure. @marionf found the issue is fixed on develop branch but in 1.7.5.0 I'm not sure

[Madalina-nadari]

ok..so i'll upgrade to 1.7.5 and see if it's working.. if not...what can I do? I really need to export my products. Can a module for export overwrite the issue ?

[matks]

@Madalina-nadari Is there something that prevents you from running your SQL query on your database ?

[Madalina-nadari]

When I make the query I wrote above, or any other one, i get the checkedform error - undefined.. The query can't be saved... I've tried the same query for another site with 1.7.0 and it's working just fine...
I am not a specialist..just trying to find an answer so I can solve this problem..

[PierreRambaud]

And what about using phpmyadmin or a tool provide by your host?

[marionf]

It's not fixed on 1.7.5.0 but it is on develop branch

[Madalina-nadari]

And what about using phpmyadmin or a tool provide by your host?

I'm not that good at that..i've tried and did not get the resultes i've wanted... I really need all the features and the images too..

It's not fixed on 1.7.5.0 but it is on develop branch

Thanks for the info..

[kershin]

I confirm: it's not fixed on 1.7.5
The only way to modify your old queries is to update entries in, for example, phpmyadmin.
Updated queries can next be used.

[espacious]

Can i know if there is a way to correct this bug with some patch because i cant update the whole prestashop.

[matks]

Can i know if there is a way to correct this bug with some patch because i cant update the whole prestashop.

Hi, basically the function RequestSql::validateSql($tab, $in, $sql) (see https://github.com/PrestaShop/PrestaShop/blob/1.7.5.x/classes/RequestSql.php#L159) checks some SQL queries and incorrectly flag them as invalid.

If you are confident in your ability to check whether a SQL request is valid and harmless you can disable the custom checks to make the function look like this:

public function validateSql($tab, $in, $sql)
    {
        if (empty($this->_errors) && !Db::getInstance()->executeS($sql)) {
            return false;
        }

        return true;
    }

(maybe using an override)

But please be aware:

• this is just a workaround, not a good solution, the right solution is to upgrade to incoming 1.7.6 release with a reworked SQL page
• if you are not a developer and do not feel confident applying the patch, please reach out for an experienced developer to help you
• this removes a safety check so you need to be careful about SQL queries you use ⚠️
• when upgrading to 1.7.6, revert this patch before in order not to create issues with the upgrading process

[espacious]

Can i know if there is a way to correct this bug with some patch because i cant update the whole prestashop.

Hi, basically the function RequestSql::validateSql($tab, $in, $sql) (see https://github.com/PrestaShop/PrestaShop/blob/1.7.5.x/classes/RequestSql.php#L159) checks some SQL queries and incorrectly flag them as invalid.

If you are confident in your ability to check whether a SQL request is valid and harmless you can disable the custom checks to make the function look like this:

public function validateSql($tab, $in, $sql)
    {
        if (empty($this->_errors) && !Db::getInstance()->executeS($sql)) {
            return false;
        }

        return true;
    }

(maybe using an override)

But please be aware:

• this is just a workaround, not a good solution, the right solution is to upgrade to incoming 1.7.6 release with a reworked SQL page
• if you are not a developer and do not feel confident applying the patch, please reach out for an experienced developer to help you
• this removes a safety check so you need to be careful about SQL queries you use ⚠️
• when upgrading to 1.7.6, revert this patch before in order not to create issues with the upgrading process

Very good explanation. Thanks. Will try to use the "patch" and upgrade as soon as possible.
Have a nice day.