Skip to content

FPM status page - JSON escape#11050

Closed
bukka wants to merge 2 commits into
php:PHP-8.1from
bukka:fpm_status_json_escape
Closed

FPM status page - JSON escape#11050
bukka wants to merge 2 commits into
php:PHP-8.1from
bukka:fpm_status_json_escape

Conversation

@bukka

@bukka bukka commented Apr 10, 2023

Copy link
Copy Markdown
Member

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.

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

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.

Comment thread ext/json/json_encoder.c Outdated
Comment thread sapi/fpm/fpm/fpm_status.c Outdated
@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
Copy Markdown
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

andypost commented May 15, 2023

Copy link
Copy Markdown
Contributor

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

bukka commented May 16, 2023

Copy link
Copy Markdown
Member Author

I will check.

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

Copy link
Copy Markdown
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