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

Set empty props as objects in WPCOM Proxy responses #2519

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Aug 13, 2024

Changes proposed in this Pull Request:

Context: p1723540799285679/1722414955.111429-slack-C02BB3F30TG

In this PR we convert empty array properties into empty objects.

Screenshots:

Screenshot 2024-08-13 at 18 47 37

Detailed test instructions:

  1. Hardcode an empty prop like $data['test'] = []; in the first line of the new prepare_data() function to simulate we get an empty array in a prop.
  2. See the prop is returned as {}
  3. Rest of the props still working as before.

Additional details:

Changelog entry

Fix - Return empty array props as empty objects in WCOM Proxy responses

@puntope puntope self-assigned this Aug 13, 2024
@puntope puntope requested review from jorgemd24 and a team August 13, 2024 16:50
@puntope puntope marked this pull request as ready for review August 13, 2024 16:50
@github-actions github-actions bot added type: bug The issue is a confirmed bug. changelog: fix Took care of something that wasn't working. labels Aug 13, 2024
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.0%. Comparing base (5ddcd31) to head (42281ed).
Report is 64 commits behind head on develop.

Files Patch % Lines
src/Integration/WPCOMProxy.php 92.9% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #2519      +/-   ##
============================================
+ Coverage       63.8%   65.0%    +1.3%     
- Complexity         0    4585    +4585     
============================================
  Files            323     475     +152     
  Lines           5052   17898   +12846     
  Branches        1222       0    -1222     
============================================
+ Hits            3222   11640    +8418     
- Misses          1664    6258    +4594     
+ Partials         166       0     -166     
Flag Coverage Δ
js-unit-tests ?
php-unit-tests 65.0% <92.9%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/Integration/WPCOMProxy.php 74.1% <92.9%> (ø)

... and 797 files with indirect coverage changes

Copy link
Contributor

@jorgemd24 jorgemd24 left a comment

Choose a reason for hiding this comment

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

Thanks @puntope for working on this. I left a comment about calling get_data and set_data without checking the $response type.

@@ -169,36 +169,59 @@ function ( $response, $handler, $request ) {
return $response;
}

$data = $response->get_data();
Copy link
Contributor

Choose a reason for hiding this comment

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

The $response can be one of these types: WP_REST_Response, WP_HTTP_Response, WP_Error, or mixed. If $response isn't a WP_REST_Response, it will throw an error because get_data or set_data might no be defined.

@puntope
Copy link
Contributor Author

puntope commented Aug 15, 2024

Thx @jorgemd24 I updated the code so now it returns early if the response is not WP_REST_Response. Can you take another look?

@puntope puntope requested a review from jorgemd24 August 15, 2024 15:49
Copy link
Contributor

@jorgemd24 jorgemd24 left a comment

Choose a reason for hiding this comment

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

Thanks @puntope for the adjustment. LGTM 👍

@puntope puntope merged commit cc158ab into develop Aug 20, 2024
12 checks passed
@puntope puntope deleted the fix/empty-props-as-objects-wpcom branch August 20, 2024 07:04
@jorgemd24 jorgemd24 mentioned this pull request Aug 20, 2024
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants