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

Fix get mime type #55

Merged
merged 3 commits into from Nov 5, 2020
Merged

Fix get mime type #55

merged 3 commits into from Nov 5, 2020

Conversation

Progi1984
Copy link
Contributor

@Progi1984 Progi1984 commented Jul 21, 2020

Questions Answers
Description? Fix get mime type
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#20254
How to test? See below

How to test ?

See "failure scenario" described at the bottom of this PR: #55 (comment)

  • failure scenario should only happen with blockreassurance v5.0.0 on PS 1.7.6 ; on PS 1.7.7 it should be fine
  • please verify that, when using this PR, the issue is solved on PS 1.7.6 and on PS 1.7.7 the issue is still not there

@Progi1984 Progi1984 changed the base branch from master to dev July 21, 2020 09:24
@Progi1984 Progi1984 requested review from matks and a team July 21, 2020 09:26
@matks
Copy link
Contributor

matks commented Jul 21, 2020

@Progi1984 We need to revert my change where I modify the compatibility scope else QA will not be able to test on 1.7.6.x.

Also we need to provide QA a specific scenario to observe the issue so they can compare before/after the PR

@Progi1984
Copy link
Contributor Author

@matks Revert the compability scope.

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR approved 👍 ready to go to QA but we need to find a failure scenario for QA, so they can assert that "with v5.0.0 it fails and with (future) v5.0.1 it works"

@reanim8ed
Copy link

reanim8ed commented Jul 22, 2020

@Progi1984 I get this error:

image
The problem occurs because PS_BASE_URI = '/'

@Progi1984 Progi1984 added the waiting for QA Status: Waiting for QA feedback label Jul 24, 2020
@matks
Copy link
Contributor

matks commented Jul 30, 2020

I have collected a reproductible scenario for QA to be able to observe and explore the bug :

  1. install blockreassurance v5.0.0 on PS 1.7.6.7

  2. modify file themes/classic/templates/catalog/product.tpl and replace line 148

{hook h='displayReassurance'}

by

{widget name="blockreassurance"}

This will now use blockreassurance as a Widget and not like a Hook.

  1. open BO configuration page for the module and edit one of the 3 reassurance items: you need to upload a custom icon

  2. now open the FO product page

  • on some environments the page will crash
  • on other environments the module will silently fail: the reassurance items will not be displayed (the block will appear empty) however you will see in Apache PHP logs the error PHP Fatal error: Uncaught Error: Call to undefined method ImageManager::getMimeType() in /modules/blockreassurance/classes/ReassuranceActivity.php:190

This should be fixed by this PR

@matks
Copy link
Contributor

matks commented Jul 30, 2020

@Progi1984 I get this error:

image
The problem occurs because PS_BASE_URI = '/'

I am sorry I dont understand this. It does not seem related to this PR, is it ?

@reanim8ed
Copy link

@Progi1984 I get this error:
image
The problem occurs because PS_BASE_URI = '/'

I am sorry I dont understand this. It does not seem related to this PR, is it ?

I found that the issue was introduced here: 37d0fb1#diff-e6316994746d2ce5be44c53e7d2527c3R190

ImageManager::getMimeType doesnt receive correct file location because str_replace returns wrong result then PS_BASE_URI = '/'.

Probably it should be registered as new issue.

@sarahdib
Copy link

sarahdib commented Sep 1, 2020

@matks
I try to reproduce the initial error but it's working without the PR for me :
Capture d’écran 2020-09-01 à 15 59 51
Capture d’écran 2020-09-01 à 16 00 22

@matks
Copy link
Contributor

matks commented Nov 4, 2020

@matks
I try to reproduce the initial error but it's working without the PR for me :
Capture d’écran 2020-09-01 à 15 59 51
Capture d’écran 2020-09-01 à 16 00 22

Is this a 1.7.6 shop ?

@Progi1984
Copy link
Contributor Author

Progi1984 commented Nov 5, 2020

@matks

failure scenario should only happen with blockreassurance v5.0.0 on PS 1.7.6 ; on PS 1.7.7 it should be fine

Yes

@sarahdib sarahdib added QA ✔️ Status: QA-Approved and removed waiting for QA Status: Waiting for QA feedback labels Nov 5, 2020
@Progi1984 Progi1984 merged commit f53b9e6 into PrestaShop:dev Nov 5, 2020
@Progi1984
Copy link
Contributor Author

Thanks @sarahdib

@Progi1984 Progi1984 deleted the fixGetMimeType branch November 5, 2020 10:44
@PierreRambaud PierreRambaud added this to the 5.1.0 milestone Nov 9, 2020
@PierreRambaud PierreRambaud added the Bug Type: Bug label Nov 9, 2020
@andromaque
Copy link

Hi @Progi1984 and @PierreRambaud,

This bug has been fixed a few months ago, targeted for version 5.1.0... but the current version is still 5.0.0.
When could we expect the release of the new version of the module?

Thank you

@KamikStudio
Copy link

KamikStudio commented May 21, 2021

@matks I have a problem with blockreassurance as well. I changed to {widget h = 'displayReassurance'}, saved the file, refreshed the cache and received a message (the one before)

ContextErrorException
.......iposomal.png): failed to open stream: No such file or directory

@KamikStudio
Copy link

@Progi1984 and when the solution?

@Frostbourn
Copy link

We need a fix.

@matks
Copy link
Contributor

matks commented Jun 18, 2021

Hello everyone, there is a huge overload of work on teams right now and many other important items requiring focus which is why v5.1.0 is not out yet. However you can use the code from dev branch to fix your module in production while waiting for the official release although it is not validated by QA (I trust you can do the necessary testing to verify the behavior 😉 )

@matks I have a problem with blockreassurance as well. I changed to {widget h = 'displayReassurance'}, saved the file, refreshed the cache and received a message (the one before)

ContextErrorException
.......iposomal.png): failed to open stream: No such file or directory

Hello @KamikStudio look at my PR #60 it might help you. I unfortunately don't have the time to continue working on it and there was a feedback from QA I could not process. You can pick up my work and even submit it as your own if you want to push the topic forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Type: Bug QA ✔️ Status: QA-Approved
Projects
None yet
9 participants