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

Remove cover query for ps1770 #96

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

pablopolyte
Copy link
Contributor

@pablopolyte pablopolyte commented Jan 19, 2021

Questions Answers
Description? Remove cover query for ps1770
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes cover image when value is "0"
How to test? Please indicate how to best verify that this PR is correct.
Possible impacts? Please indicate what parts of the software we need to check to make sure everything is alright.

@matks matks changed the title remove cover query for ps1770 Remove cover query for ps1770 Jan 20, 2021
@matks
Copy link
Contributor

matks commented Jan 20, 2021

Hi @pablopolyte thanks for the PR! Can you give us some context? Why do you suggest to remove the cover query for PS 1770? Is there a change in PS 1.7.7 that requires this?

@kpodemski
Copy link
Contributor

It's probably because we're able to get proper cover from .tpl, but I don't see changes related to this

@sarahdib sarahdib self-assigned this Jan 21, 2021
@sarahdib
Copy link

@matks & @kpodemski it's because I found a bug on the new release of blockwishlist
#77 (comment)
This PR fix the first bug :)

@kpodemski
Copy link
Contributor

@sarahdib ok, if so, this is not a solution, of course this code could be removed by:

@Quetzacoalt91
Copy link
Member

Quetzacoalt91 commented Jan 21, 2021

Hello !

We discovered that with one of the wishlist SQL requests, we could get a 0 when no image is chosen for a given combination.

The core is able to provide it by itself only if we DO NOT provide this information. As soon as we give a value, even wrong, it does nothing for us:
https://github.com/PrestaShop/PrestaShop/blob/6a880d31539ab2dcad5be32d7cdc332574a4fe43/classes/Product.php#L5494-L5499

Because of this value zero, it appears that we enter this if() block and make the core failing: https://github.com/PrestaShop/PrestaShop/blob/6a880d31539ab2dcad5be32d7cdc332574a4fe43/src/Adapter/Presenter/Product/ProductLazyArray.php#L606-L622

By leaving the core handling it completely, we get rid of this error.

@kpodemski
Copy link
Contributor

@Quetzacoalt91 sure, I understand, make sure to check this PR tho:
#89

As without it y'all make sure to edit .tpl inside a module to get cover based on combination added to wishlist, not default one.

$querySearch->select('
(SELECT `id_image` FROM `' . _DB_PREFIX_ . 'product_attribute_image` WHERE `id_product_attribute` = product_attribute_shop.`id_product_attribute`) cover_image_id'
);

This comment was marked as outdated.

This comment was marked as outdated.

@Quetzacoalt91
Copy link
Member

I see. Because we can't change the behavior on already released versions of PrestaShop, I guess one workaround could be found on the module side.
I'm not sure if we were supposed to find zeros in the column product_attribute_image.id_image, but we have to ask MySQL to return null when this happens.

Would the suggestion above this comment help? ⬆️

@kpodemski
Copy link
Contributor

@Quetzacoalt91

you can safely remove this query :) to avoid issues with cover:

This should be enough.

@Quetzacoalt91
Copy link
Member

Quetzacoalt91 commented Jan 21, 2021

Oh okay ! I didn't get on the last message the changes were needed on the template.

Ping @pablopolyte

@NeOMakinG NeOMakinG merged commit 7b607db into PrestaShop:new-module Jan 25, 2021
@NeOMakinG NeOMakinG added this to the 2.0.0 milestone Jan 25, 2021
@kpodemski kpodemski mentioned this pull request Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants