Skip to content
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

Fix Message::bodySummary when preg_match fails #554

Merged
merged 2 commits into from Apr 17, 2023

Conversation

XilefNori
Copy link
Contributor

preg_replace returns "false" on failure which interpreted as OK result which is wrong.
Reform statement in positive manner makes it right

Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

I think the right fix here is just to add a === 0?

preg_replace returns "false" on failure which interpreted as OK result which is wrong.

Reform statement in positive manner makes it right
@XilefNori
Copy link
Contributor Author

I think the right fix here is just to add a === 0?

I guess you right

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

❌ If we did as you suggest, the function might return non-printable bytes when preg_match is “broken”. Note that the documentation specifically says “Will return null if the response is not printable.” and we just cannot tell without preg_match.

By the way, how does preg_match even break?

@XilefNori
Copy link
Contributor Author

XilefNori commented Apr 17, 2023

By the way, how does preg_match even break?

For example when you has utf-8 malformed symbol in tested string (which you get when cut utf-8 string code in the middle of multibyte UTF character).

❌ If we did as you suggest, the function might return ç when preg_match is “broken”. Note that the documentation specifically says “Will return null if the response is not printable.” and we just cannot tell without preg_match.

No, it will not. When preg_match breaks you get false (!== 0), therefore it will return null.

if (preg_match('/[^\pL\pM\pN\pP\pS\pZ\n\r\t]/u', $summary) === 0) {
    return $summary;
}

return null;

@GrahamCampbell GrahamCampbell changed the title fix Message::bodySummary when preg_replace is broken Fix Message::bodySummary when preg_match fails Apr 17, 2023
@GrahamCampbell GrahamCampbell changed the base branch from master to 2.4 April 17, 2023 13:11
@GrahamCampbell GrahamCampbell merged commit 3f849aa into guzzle:2.4 Apr 17, 2023
17 checks passed
@jtojnar
Copy link
Contributor

jtojnar commented Apr 17, 2023

No, it will not. When preg_match breaks you get false (!== 0), therefore it will return null.

Right, but that is precisely what I would expect based on “Will return null if the response is not printable.” UTF-8 malformed symbol is not printable.

@jtojnar
Copy link
Contributor

jtojnar commented Apr 17, 2023

Turns out I was misunderstanding you. It was previously returning a malformed UTF-8 and your change makes it return null. I added a test: #559

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

Successfully merging this pull request may close these issues.

None yet

3 participants