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

CO: Fix misuse of getimagesize returned informations #11809

Merged
merged 2 commits into from Dec 21, 2018

Conversation

Projects
None yet
5 participants
@olivier-monaco
Copy link
Contributor

olivier-monaco commented Dec 18, 2018

Questions Answers
Branch? develop
Description? Cherry pick of #6128 on develop.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no

This change is Reviewable

@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Dec 18, 2018

Hello @olivier-monaco!

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

// Evaluate the memory required to resize the image: if it's too much, you can't resize it.
// For perfs, avoid computing static maths formulas in the code. pow(2, 16) = 65536 ; 1024 * 1024 = 1048576
if (($infos[0] * $infos[1] * $infos['bits'] * $channel + 65536) * 1.8 + $currentMemory > $memoryLimit - 1048576) {
if (($infos[0] * $infos[1] * $bits * $channel + 65536) * 1.8 + $current_memory > $memoryLimit - 1048576) {

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Dec 18, 2018

Member

You replaced $currentMemory by $current_memory. This will throw errors about an undefined variable because its name changed.

This comment has been minimized.

@olivier-monaco

olivier-monaco Dec 18, 2018

Contributor

Ho, I missed it... sorry.

This comment has been minimized.

@olivier-monaco

olivier-monaco Dec 18, 2018

Contributor

Fixed with last commit

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Dec 18, 2018

@olivier-monaco, can you please take some time to fill the table in your first message? This helps us to classify you PR properly and the QA team to test the changes.

Thanks by advance

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

olivier-monaco commented Dec 18, 2018

@Quetzacoalt91 already done ;). But I was surprised to have an issue I fixed in 1.6.1.x...

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Dec 18, 2018

I understand & agree, this should have been cherry picked on develop.

@ntiepresta ntiepresta self-assigned this Dec 20, 2018

@ntiepresta

This comment has been minimized.

Copy link

ntiepresta commented Dec 20, 2018

@olivier-monaco
Hi, please, can you give us more the information for the test QA. To better meet you needs, please can you provide us the screenshot or a video record of the issue.
Regards!

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

olivier-monaco commented Dec 20, 2018

Hi @ntiepresta,
I don't understand what you need. As said, this is just a cherry-pick of #6128: a fix I've done for 1.6 and I just discovered yesterday it was never applied to 1.7.

From the original PR:

Some misuse of getimagesize returned informations:

  • from documentation, the "bits" entry can be missing (so added a check on it)
  • the number of channels were divide by 8 and the number of bits used as is... the final value was the same when the channels entry is present but not when it is missing.

So, it's just a "follow the doc" and a "do computation the right way". There is no screenshot or video.

@ntiepresta

This comment has been minimized.

Copy link

ntiepresta commented Dec 20, 2018

@olivier-monaco
Thanks for your reply.
Regards!

@ntiepresta ntiepresta added QA ✔️ and removed waiting for QA labels Dec 20, 2018

@ntiepresta ntiepresta removed their assignment Dec 21, 2018

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.6.0 milestone Dec 21, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit da15687 into PrestaShop:develop Dec 21, 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
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Dec 21, 2018

Thank you @olivier-monaco

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