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 WebserviceOutputBuilder - handle api categories blank schema #28540

Merged
merged 3 commits into from
Nov 3, 2022

Conversation

angelo983
Copy link
Contributor

@angelo983 angelo983 commented May 19, 2022

Validation of category association id

Questions Answers
Branch? develop
Description? Resolve 'Trying to access array offset on value of type null' in calling api categories blank schema
Type? bug fix
Category? WS
BC breaks? no
Deprecations? yes
Fixed ticket? Fixes #28543
How to test? Follow above ticket steps. Call a blank schema of Categories in a shop with Categories tree level depth > 1
Possible impacts? Categories API

Validation of category association id
@angelo983 angelo983 requested a review from a team as a code owner May 19, 2022 14:32
@prestonBot
Copy link
Collaborator

Hello @angelo983!

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

@prestonBot prestonBot added the Bug fix Type: Bug fix label May 19, 2022
@angelo983
Copy link
Contributor Author

Fixes #28543

@prestonBot prestonBot added the develop Branch label May 19, 2022
matks
matks previously approved these changes May 19, 2022
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.

Dear @angelo983 thank you for the PR!

Code seems OK, just need to wait for confirmation of bug #28543

@matks matks changed the title Update WebserviceOutputBuilder.php Fix WebserviceOutputBuilder - handle api categories blank schema May 19, 2022
@angelo983
Copy link
Contributor Author

Hello @matks, you are welcome!
I pushed another commit covering more cases.

Progi1984
Progi1984 previously approved these changes May 23, 2022
matks
matks previously approved these changes May 23, 2022
@matks matks added the Waiting for QA Status: action required, waiting for test feedback label May 23, 2022
Comment on lines 668 to 670
if (!empty($value['id'])) {
$fields_assoc = [['id' => $value['id']]];
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you please use AND operator instead a new IF statement ? (see the line just bellow)

@angelo983 angelo983 dismissed stale reviews from matks and Progi1984 via c6e8b38 May 23, 2022 14:05
@angelo983
Copy link
Contributor Author

@PululuK I did what you asked for

@AureRita AureRita self-assigned this May 24, 2022
@AureRita
Copy link
Contributor

Hi @angelo983

Thank you for your PR but, currently, the issue that you want to correct didn't exist on develop and 178x, so they're already corrected, as you can see :

on 1.7.8.x :
image

on old develop ( 3 month ago I think ) :
image

on new develop ( with an other PR ) :
image

If you take the files from the 1.7.8.4 version, I test it and yes, it correct the issue !

@angelo983
Copy link
Contributor Author

angelo983 commented May 24, 2022

@AureRita you replicated yourself the issue #28543.
Do you want to tell me that in version 1.7.8.6 the problem does not arise?
What number is the other PR you mentioned?

@AureRita
Copy link
Contributor

Hi @angelo983 ,

Yes, that's it, the problem does not arise in version 1.7.8.6

I mentioned a PR that I tested near this one, if you want more information it was pr-27755

@AureRita AureRita added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels May 25, 2022
@angelo983
Copy link
Contributor Author

angelo983 commented May 27, 2022

@AureRita in 1.7.8.6 the bug is still present, in my develop branch not because my patch is present.
error

@AureRita
Copy link
Contributor

Hi @angelo983,

I do a video for you on develop without your pr :

on.develop.mp4

And another on version 1.7.8.6 :

on.1786.mp4

Can you do a video or anything else to see this problem on 1.7.8.6, it's maybe a problem with some restriction on your computer ?

Why do your webservice get blue character ?

@MatShir
Copy link
Contributor

MatShir commented Aug 18, 2022

Hey @angelo983,
We are planning a patch release for September, it would be nice to have your fix 🚀 Could you please check last @AureRita message ?

@atomiix
Copy link
Contributor

atomiix commented Sep 5, 2022

@angelo983 friendly reminder :)

@angelo983
Copy link
Contributor Author

Hello,
I have reproduced the problem also in 1.7.8.6 following what I wrote in #28543 issue.

I have done my part, if someone wants to go deeper could check why and how an empty value can arrive to objectAssoc when renderAssociations function is called.

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.

Hi everyone I'm lost 😅 what's happening with this pull request?

@kpodemski kpodemski added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback labels Oct 28, 2022
@kpodemski
Copy link
Contributor

Hello @AureRita

You reproduced the issue before, but later you couldn't, could you please double-check if the issue is not there anymore? We have information from @angelo983 that the bug is still there.

Copy link
Contributor

@AureRita AureRita left a comment

Choose a reason for hiding this comment

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

Hi @angelo983 !

Sorry for that long long time, after many research I see where is the differents to repeat the issue ( we need to disabled the debug mode ). After checking this, I try your PR and it's seems to works !

we can see this without your pr :

Untitled_.Oct.31.2022.2_42.PM.webm

And with your PR :

Untitled_.Oct.31.2022.2_43.PM.webm

Thank you for your Patience and your PR !

@AureRita AureRita added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Oct 31, 2022
@prestashop-issue-bot
Copy link

QA OK without required approvals !? :trollface:

@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@Progi1984 Progi1984 added this to the 8.1.0 milestone Oct 31, 2022
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @angelo983

@jolelievre
Copy link
Contributor

The PR was tested and approved by QA, but it's missing one review approval (previous approvals were removed after handling a suggestion) Just waiting for the second approval to merge this one

@AureRita AureRita removed their assignment Nov 3, 2022
@kpodemski kpodemski merged commit b74ad49 into PrestaShop:develop Nov 3, 2022
@kpodemski
Copy link
Contributor

Thank you @angelo983 :)

@angelo983 angelo983 deleted the patch-1 branch November 3, 2022 13:59
@matteolavaggi
Copy link

Why this bug is not fixed in PS 1.7.8.8 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix Type: Bug fix develop Branch QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'Trying to access array offset on value of type null' calling WS api categories blank schema