Skip to content

Fix FPM status json encoded value test#11276

Closed
bukka wants to merge 1 commit into
php:PHP-8.1from
bukka:fpm_status_json_field_test
Closed

Fix FPM status json encoded value test#11276
bukka wants to merge 1 commit into
php:PHP-8.1from
bukka:fpm_status_json_field_test

Conversation

@bukka

@bukka bukka commented May 19, 2023

Copy link
Copy Markdown
Member

This should fix the failure when incorrect process is selected. As it is a static pool, it can be in any process so we cannot expect that process 0 is the query one.

@bukka
bukka changed the base branch from master to PHP-8.1 May 19, 2023 12:21
@bukka
bukka force-pushed the fpm_status_json_field_test branch from 95e64ca to 5178b84 Compare May 19, 2023 12:21
@bukka

bukka commented May 19, 2023

Copy link
Copy Markdown
Member Author

@iluuu1994 That should fix the failure that you reported.

@andypost Not sure however if this is going to fix the issue that you reported so if you could give it a try that would be appreciated.

@bukka
bukka force-pushed the fpm_status_json_field_test branch from 5178b84 to e3d11fd Compare May 19, 2023 14:16
@andypost

Copy link
Copy Markdown
Contributor

Fix 5178b84 failed as CI

just re-queued e3d11fd

@iluuu1994 iluuu1994 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.

LGTM otherwise

);
}
foreach ($data['processes'] as $process) {
if (preg_match('/' . $pattern . '/', $process[$fieldName]) !== false) {

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.

I recommend using () as the delimiter so that the caller doesn't need to escape / or any other literal character.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it for compatibility with Windows?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah yeah will use pipes in case it was used for matching the whole uri...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm actually using pipes elsewhere - not sure why I used slashes here...

@andypost

Copy link
Copy Markdown
Contributor

Confirm the last change fixed the test

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.

3 participants