Skip to content

FPM status page - JSON escape #11050

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

Closed
wants to merge 2 commits into from
Closed

Conversation

bukka
Copy link
Member

@bukka bukka commented Apr 10, 2023

This fixes bug #64539. It exposes the JSON string escape wrapped in the new API function php_json_encode_string. That is then used in the status page for each query string.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Looks sensible to me, just minor comments.

@bukka bukka force-pushed the fpm_status_json_escape branch from c915360 to a34f93c Compare April 10, 2023 15:26
@bukka bukka force-pushed the fpm_status_json_escape branch from a34f93c to 78fb2fd Compare May 12, 2023 21:41
@bukka bukka closed this in 5e64ead May 13, 2023
@iluuu1994
Copy link
Member

@bukka There were some failures in nightly in regards to this change.

https://github.com/php/php-src/actions/runs/4969852729/jobs/8893356141

 ========DIFF========
001- string(5) "a=b"c"
001+ string(9) "full&json"
     Done
========DONE========
FAIL FPM: bug64539 - status json format escaping [sapi/fpm/tests/bug64539-status-json-encoding.phpt]

@andypost
Copy link
Contributor

andypost commented May 15, 2023

This test constantly fails on Alpinelinux for all 32-bit arches (x86,armhf, armv7)

TEST 16161/16323 [sapi/fpm/tests/bug64539-status-json-encoding.phpt]
========DIFF========
- string(5) "a=b"c"
+ string(0) ""
     Done
========DONE========

Ref https://gitlab.alpinelinux.org/andypost/aports/-/merge_requests/3#note_307466

@bukka
Copy link
Member Author

bukka commented May 16, 2023

I will check.

iluuu1994 added a commit that referenced this pull request May 17, 2023
@iluuu1994
Copy link
Member

@bukka I marked the test as XFAIL for now, feel free to remove it whenever you have the time to have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants