Skip to content

Fix FPM status json encoded value test #11276

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 1 commit into from

Conversation

bukka
Copy link
Member

@bukka bukka commented May 19, 2023

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
Copy link
Member Author

bukka commented May 19, 2023

@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
Contributor

Fix 5178b84 failed as CI

just re-queued e3d11fd

Copy link
Member

@iluuu1994 iluuu1994 left a comment

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
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
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
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
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
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