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

Add DB prefix in delete() method all time $add_prefix is set to true #8329

Merged
merged 1 commit into from Nov 23, 2017

Conversation

Projects
None yet
8 participants
@prestamodule
Contributor

prestamodule commented Sep 14, 2017

Questions Answers
Branch? 1.6.1.x
Description? While you set up your shop with the DB prefix as "pr", you will have some errors occuring while using the delete() method of Db class cause this one will try to include the prefix only and only if this one do not start the name of the table. Eg: pr is the beginning of products, so while you try a delete() on products table, it will be an error (the prefix will not be included).
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
How to test? Install a shop with "pr" DB prefix and made a delete() on table "products" with/without this change.

Eg: Set the shop with a DB prefix as this one: cu
Try to make this call: Db::getInstance()->delete('customer_group', 'id_customer = 1', 1);
The delete() method will start like that: DELETE FROM customer_group

Now, set the show with a Db Prefix as this one: cu_
The same call and you will have that: DELETE FROM cu_customer_group


This change is Reviewable

@Quetzacoalt91 Quetzacoalt91 added this to the 1.6.1.18 milestone Sep 18, 2017

@Quetzacoalt91 Quetzacoalt91 merged commit ed5241b into PrestaShop:1.6.1.x Nov 23, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SharakPL

This comment has been minimized.

Show comment
Hide comment
@SharakPL

SharakPL Feb 15, 2018

Sorry but I'm not so good with Git yet, so I'll just comment here.
I believe this should be reverted, as it causes errors in modules using delete() function, i.e. themeconfigurator module in prestashop 1.6.1.18. My db_prefix is om_ (default is ps_ so it should be all good using underscore). While trying to remove items in themeconfigurator I get a DB error, because it's trying to update table om_om_themeconfigurator. Worked just fine in 1.6.1.17 with conditions if (_DB_PREFIX_ && !preg_match('#^'._DB_PREFIX_.'#i', $table) && $add_prefix)

SharakPL commented Feb 15, 2018

Sorry but I'm not so good with Git yet, so I'll just comment here.
I believe this should be reverted, as it causes errors in modules using delete() function, i.e. themeconfigurator module in prestashop 1.6.1.18. My db_prefix is om_ (default is ps_ so it should be all good using underscore). While trying to remove items in themeconfigurator I get a DB error, because it's trying to update table om_om_themeconfigurator. Worked just fine in 1.6.1.17 with conditions if (_DB_PREFIX_ && !preg_match('#^'._DB_PREFIX_.'#i', $table) && $add_prefix)

@PrestaEdit

This comment has been minimized.

Show comment
Hide comment
@PrestaEdit

PrestaEdit Feb 15, 2018

Contributor

Surely not. No revert on this, or it will never work great.

But, yes, the themeconfigurator module need to be update. The DB_PREFIX is not needed (and we need to avoid it !) inside delete() method. Or need to pass false to $add_prefix param.

But, I agree, an update of this module is needed.

Contributor

PrestaEdit commented Feb 15, 2018

Surely not. No revert on this, or it will never work great.

But, yes, the themeconfigurator module need to be update. The DB_PREFIX is not needed (and we need to avoid it !) inside delete() method. Or need to pass false to $add_prefix param.

But, I agree, an update of this module is needed.

@SharakPL

This comment has been minimized.

Show comment
Hide comment
@SharakPL

SharakPL Feb 15, 2018

So to be clear. All instances of delete(_DB_PREFIX_.'some_module_table', 'id_item = '.(int)$id_item) should now be changed to delete('some_module_table', 'id_item = '.(int)$id_item). Correct?

SharakPL commented Feb 15, 2018

So to be clear. All instances of delete(_DB_PREFIX_.'some_module_table', 'id_item = '.(int)$id_item) should now be changed to delete('some_module_table', 'id_item = '.(int)$id_item). Correct?

@PrestaEdit

This comment has been minimized.

Show comment
Hide comment
@PrestaEdit

PrestaEdit Feb 15, 2018

Contributor

For me, yes. It's better.

Contributor

PrestaEdit commented Feb 15, 2018

For me, yes. It's better.

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Feb 16, 2018

Contributor

Agree with prestamodule, this a standard in this class that we don't need to provide prefix

Contributor

kpodemski commented Feb 16, 2018

Agree with prestamodule, this a standard in this class that we don't need to provide prefix

@daresh

This comment has been minimized.

Show comment
Hide comment
@daresh

daresh Mar 29, 2018

This is a very bad solution, causes some modules to break (the ones that use DB_PREFIX in delete method, for example the Themeconfigurator module).

daresh commented on 935d85f Mar 29, 2018

This is a very bad solution, causes some modules to break (the ones that use DB_PREFIX in delete method, for example the Themeconfigurator module).

This comment has been minimized.

Show comment
Hide comment
@camlafit

camlafit Apr 17, 2018

Contributor

Hello

I confirm it's a bad solution some modules are broken as dpdfrance.
Should be reverted or add_prefix set to false by default.

At least doesn't set in a minor version

Contributor

camlafit replied Apr 17, 2018

Hello

I confirm it's a bad solution some modules are broken as dpdfrance.
Should be reverted or add_prefix set to false by default.

At least doesn't set in a minor version

camlafit pushed a commit to Webelys-PS/DPDFrance_Prestashop1.0-1.6 that referenced this pull request Apr 17, 2018

Camille Lafitte
Force prefix on delete action
* Follow change pushed in 1.6.1.18
PrestaShop/PrestaShop#8329
* To allow delete action we must set add_prefix to false.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment