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

Check if key exists before checking the value #9105

Merged
merged 2 commits into from Jun 5, 2018

Conversation

Projects
None yet
7 participants
@andypieters
Contributor

andypieters commented May 22, 2018

Questions Answers
Branch? develop
Description? Our plugin uses a virtual product to add a fee to the order. Before calculating the fee, i make sure the fee product is removed from the cart (and added afterwards). FYI this is the code in my plugin that handles this. When developer mode is active, we get a notice on the line i just edited, halting the checkout process. Notice: Undefined index: 31- in Cart.php line 1653
Type? bug fix
Category? CO
BC breaks? No
Deprecations? No
Fixed ticket? N.A.
How to test? I think the fix is quite obvious

Got some complaints from our customers about this.
Our plugin manages a 'fee' product, but when we try to delete the product, a notice is shown.
It seems like you should check if the value is set before checking what the value is.

type: bug fix
CO: Check if key exists before checking the value when removing preservedGifts from cart


This change is Reviewable

@prestonBot

This comment has been minimized.

Collaborator

prestonBot commented May 22, 2018

Hello @andypieters!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented May 22, 2018

Hi @andypieters,

Would you mind filling the description table? They are used to classify the contributions on the project.
I brought it back on your first message, you just have to edit it.

Thanks!

@prestonBot prestonBot added the develop label May 23, 2018

@mickaelandrieu mickaelandrieu added the Bug label May 23, 2018

@Quetzacoalt91

@andypieters it seems your fix may hide another issue. Can you please have a look at my code and confirm it also fixes the issue?

Thanks

@@ -1650,7 +1650,7 @@ public function deleteProduct(
}
$preservedGifts = $this->getProductsGifts($id_product, $id_product_attribute);
if ($preservedGifts[$id_product.'-'.$id_product_attribute] > 0) {
if (isset($preservedGifts[$id_product.'-'.$id_product_attribute]) && $preservedGifts[$id_product.'-'.$id_product_attribute] > 0) {

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 May 24, 2018

Member

Actually, a better would be a integer cast of the variable $id_product_attribute. The isset() here may prevent us to get the gifts in some case, although it exists. If you check the content of getProductsGifts(), you will see the parameter is casted ((int)null = 0).

        $preservedGifts = $this->getProductsGifts($id_product, $id_product_attribute);
        if ($preservedGifts[$id_product.'-'. (int)$id_product_attribute] > 0) {
            return Db::getInstance()->execute(
                'UPDATE `'._DB_PREFIX_.'cart_product`
                SET `quantity` = '.(int)$preservedGifts[$id_product.'-'.(int)$id_product_attribute].'
                WHERE `id_cart` = '.(int)$this->id.'
                AND `id_product` = '.(int)$id_product.
                ($id_product_attribute != null ? ' AND `id_product_attribute` = '.(int)$id_product_attribute : '')
            );
        }

This caused by the default value NULL in the function parameters, although we expect a integer...
BTW, why the method getProductsGifts return an array while a integer would be enough.

Casting preservedGifts id_product_attribute to int
To prevent errors when id_product_attribute is null
@andypieters

This comment has been minimized.

Contributor

andypieters commented May 25, 2018

@Quetzacoalt91 Yes this fixes it too.
I've updated the pull request

@marionf

This comment has been minimized.

Contributor

marionf commented May 28, 2018

@eternoendless 1.7.4.1 ?

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 5, 2018

Thanks @andypieters !

Ping @eternoendless, let me know if this need to be cherry-picked

@mickaelandrieu mickaelandrieu merged commit 572c390 into PrestaShop:develop Jun 5, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eternoendless eternoendless added this to the 1.7.5.0 milestone Jun 6, 2018

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