Sync: silence the call to `gzinflate` #34186

bor0 · 2023-11-17T12:41:12Z

Proposed changes:

Silence the call to gzinflate:

Can't always assume that the input is a valid compressed string
There are no functions that we can use to check if the data is valid
WordPress core also uses @gzinflate.

Should fix a few warnings :)

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Does this pull request change what data or activity we track or use?

No

Testing instructions:

N/A

- Can't always assume that the input is a valid compressed string - There are [no functions](https://www.php.net/manual/en/function.gzinflate.php) that we can use to check if the data is valid - WordPress core also uses `@gzinflate`.

github-actions · 2023-11-17T12:42:09Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the silence-inflate branch.
To test on Simple, run the following command on your sandbox:
```
bin/jetpack-downloader test jetpack silence-inflate
```

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2023-11-17T12:42:14Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

anomiex · 2023-11-17T18:13:08Z

projects/packages/sync/src/class-json-deflate-array-codec.php

@@ -41,7 +41,7 @@ public function encode( $object ) {
 	 * @return array|mixed|object
 	 */
 	public function decode( $input ) {
-		return $this->json_unserialize( gzinflate( base64_decode( $input ) ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode
+		return $this->json_unserialize( @gzinflate( base64_decode( $input ) ) ); // phpcs:ignore


Let's not use generic phpcs:ignore. Better to be clear about what exactly we're ignoring.

Also, it seems like it might make more sense to store the result of gzinflate in a variable and check for false so we could skip trying to json_unserialize that.

Thank you! I agree. Addressed these in 436ac0e.

…serialize

This PR further extends the functionality of a previous PR php#4985. Ran into this while working on a [particular WordPress functionality](Automattic/jetpack#34186) - our servers reported tons of warnings. Currently, the only way to avoid warnings is to call `@gzinflate`, but silencing errors is discouraged so we also have to `phpcs:ignore` it. If `gzinflate` threw an exception instead, it would be much easier for developers to handle this. Applied same reasoning to all the zlib functions. I also found out that the `json` extension also does not use any notices: ``` bor0:~/dev/php-src/ext/json$ grep -R php_error_docref * bor0:~/dev/php-src/ext/json$ ``` ⚠️This is a breaking change and may cause code flow interruptions unless handled with exceptions⚠️

fgiannar

LGTM 👍

Apologies for the late review. Give me a ping to re-approve after resolving conflicts if needed

fgiannar

LGTM

Silence the call to gzinflate

4e699aa

- Can't always assume that the input is a valid compressed string - There are [no functions](https://www.php.net/manual/en/function.gzinflate.php) that we can use to check if the data is valid - WordPress core also uses `@gzinflate`.

bor0 requested a review from fgiannar November 17, 2023 12:41

github-actions bot added the [Package] Sync label Nov 17, 2023

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 17, 2023

bor0 added [Action] Required Review [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Team Review labels Nov 17, 2023

github-actions bot removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Nov 17, 2023

bor0 removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 17, 2023

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 17, 2023

Add changelog

378c41a

bor0 removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 17, 2023

bor0 added 2 commits November 17, 2023 13:52

Make phpcs happy

6bbe400

Use correct package version

7e24190

fgiannar added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Nov 17, 2023

anomiex reviewed Nov 17, 2023

View reviewed changes

Be specific about phpcs:ignore and avoid unnecessary calls to json_un…

436ac0e

…serialize

bor0 mentioned this pull request Nov 17, 2023

Convert more zlib warnings to exceptions php/php-src#12708

Closed

jeherve changed the title ~~Silence the call to gzinflate~~ Sync: silence the call to gzinflate Nov 20, 2023

fgiannar previously approved these changes Nov 23, 2023

View reviewed changes

fgiannar added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Team Review labels Nov 23, 2023

bor0 dismissed fgiannar’s stale review via a857c24 November 23, 2023 12:59

fgiannar previously approved these changes Nov 23, 2023

View reviewed changes

fgiannar added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 23, 2023

Merge branch 'trunk' into silence-inflate

bf93f76

bor0 dismissed fgiannar’s stale review via bf93f76 November 23, 2023 13:06

bor0 force-pushed the silence-inflate branch from a857c24 to bf93f76 Compare November 23, 2023 13:06

bor0 enabled auto-merge (squash) November 24, 2023 13:24

Merge branch 'trunk' into silence-inflate

003dd4b

fgiannar approved these changes Nov 24, 2023

View reviewed changes

bor0 merged commit 8cd57bf into trunk Nov 24, 2023
60 checks passed

bor0 deleted the silence-inflate branch November 24, 2023 13:45

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync: silence the call to `gzinflate` #34186

Sync: silence the call to `gzinflate` #34186

bor0 commented Nov 17, 2023 •

edited

github-actions bot commented Nov 17, 2023 •

edited

github-actions bot commented Nov 17, 2023 •

edited

anomiex Nov 17, 2023

bor0 Nov 17, 2023

fgiannar left a comment

fgiannar left a comment

Sync: silence the call to gzinflate #34186

Sync: silence the call to gzinflate #34186

Conversation

bor0 commented Nov 17, 2023 • edited

Proposed changes:

Other information:

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Nov 17, 2023 • edited

github-actions bot commented Nov 17, 2023 • edited

anomiex Nov 17, 2023

Choose a reason for hiding this comment

bor0 Nov 17, 2023

Choose a reason for hiding this comment

fgiannar left a comment

Choose a reason for hiding this comment

fgiannar left a comment

Choose a reason for hiding this comment

Sync: silence the call to `gzinflate` #34186

Sync: silence the call to `gzinflate` #34186

bor0 commented Nov 17, 2023 •

edited

github-actions bot commented Nov 17, 2023 •

edited

github-actions bot commented Nov 17, 2023 •

edited