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

[ps_accounts] Wrong condition #12

Closed
d0niek opened this issue Mar 30, 2023 · 8 comments
Closed

[ps_accounts] Wrong condition #12

d0niek opened this issue Mar 30, 2023 · 8 comments
Assignees
Labels
question Further information is requested

Comments

@d0niek
Copy link

d0niek commented Mar 30, 2023

Hi!

It looks like you have wrong conditions in file ps_accounts.php, on line 528.

It looks:

if (!$response || true !== $response['status']) {

and I think it should looks like:

if ($response && true !== $response['status']) {

My installed version is 5.3.4.

@emmanuelgautier emmanuelgautier added the question Further information is requested label Mar 30, 2023
@emmanuelgautier
Copy link
Contributor

Hi @d0niek,

The first condition looks good. The line condition catches two potential errors which are no response object or a response status with an error. Your proposition won't catch the case when there is no response object.

Do you encounter a specific issue with this line?

@d0niek
Copy link
Author

d0niek commented Mar 30, 2023

Hi @emmanuelgautier!

Good point of view!

So if you want to catch "no response object" then you can't use "that object" on line 530 and 531.

@emmanuelgautier
Copy link
Contributor

You are right, we will fix that. Thanks for your report.

Having no $response should not happen btw. Did you encounter a specific issue with an empty response? If so, could you share with us some context in order to better understand your use case?

@d0niek
Copy link
Author

d0niek commented Mar 30, 2023

I got this when I enable new theme, just after added it to the shop.

I did this 3, 4 times (fresh install on dev or restore backup on test environment) and for sure 2, 3 times I got an error. I run with debug on so script stop and nice exception output got from Symfony but when I go to shop page, new theme was loaded and everything was working.

I worked with phalcone and warehouse themes.

My setup is:
Prestashop 1.7.8.8
Php 7.4.30+

@emmanuelgautier
Copy link
Contributor

I @d0niek,

A new ps_accounts version 5.3.5 has been published today. This new version should be available for download in the coming hours.

Do not hesitate to get back to us if you have the opportunity to test this new version.

Thanks again for raising this issue and contributing to improving Prestashop modules.

@d0niek
Copy link
Author

d0niek commented Apr 7, 2023

Hi @emmanuelgautier !

Code looks like:

if (!$response || true !== $response['status']) {
    $this->getLogger()->debug(
        'Error trying to PATCH shop : ' . $response['httpCode'] .
        ' ' . print_r($response['body']['message'], true)
    );
}

I think you can't check if $response is empty and then use $response['some_key']. For sure 'some_key' doesn't exists, $response is empty (!).

I thought that condition is wrong but you said that you want to catch empty $response. If so you have to log different message for that, without using this object in the log message.

@emmanuelgautier
Copy link
Contributor

Hi @d0niek

The changes have been applied to the newer version of the module (6.x) but we forgot to backport them to the 5.x branch. A new release has been published today and should be available tomorrow.

Sorry for the inconvenience and thanks for your patience.

@emmanuelgautier
Copy link
Contributor

A fix for this issue has been released. This issue can be closed. Thanks for having created this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants