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

Encoding malformed characters #80

Merged
merged 1 commit into from Apr 16, 2024
Merged

Encoding malformed characters #80

merged 1 commit into from Apr 16, 2024

Conversation

4513
Copy link
Contributor

@4513 4513 commented Apr 11, 2024

No collected data available

While profiling an application, malformed data were sent to a saver (both, Upload and File) that called json_encode with the provided data.
That resulted into an encoding error, returning false that was casted into an empty string.

Because of that, no collected data was available at the end of the request.

The proposed change uses JSON_INVALID_UTF8_IGNORE constant which removes all invalid characters, making the encoding possible.

The constant is available since PHP 7.2, that is why a 'check' is made before the encoding. If the PHP has the constant, it will use it.


Even though the change removes a word/key from the resulted output, the change does not have any undesirable changes because the previous (current) behavior does not generate the output at all.

@glensc
Copy link
Contributor

glensc commented Apr 11, 2024

Can you refactor this using a single constant PHP_VERSION_ID check and setting $options rather than multiple json_encode calls? (default is $flags = 0 - https://www.php.net/json_encode)

Example:

@glensc
Copy link
Contributor

glensc commented Apr 11, 2024

also, you should never create PR from default branch (always create feature branch), making changes for the PR would mess up your default branch and vice versa. you might want to keep them separate.

@glensc
Copy link
Contributor

glensc commented Apr 11, 2024

also, while at it, fix Mlaformed -> malformed typo in commit message.


return file_put_contents($this->file, $json . PHP_EOL, FILE_APPEND);
return $result === false ? '' : $result;
Copy link
Contributor

@glensc glensc Apr 11, 2024

Choose a reason for hiding this comment

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

why put empty line to file? maybe skip whole write then?

*/
private function encodeData(array $data)
{
$result = PHP_MAJOR_VERSION >= 7 && PHP_MINOR_VERSION >= 2
Copy link
Contributor

Choose a reason for hiding this comment

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

this check fails with php 8.0, use PHP_VERSION_ID instead

@4513
Copy link
Contributor Author

4513 commented Apr 12, 2024

  • changed to use PHP_VERSION_ID instead of the previous two constants
  • changed to rather setting $option than calling json_econde differently
  • added condition to check that the result of json_encode is valid. If false is returned, the save is immediately stopped with return 'false' (no saving into file or sending a request to upload the json)

Please, note that I created a new branch as noted on which the proposed changes are made, including fixed typo in commit message.

If you are okay, with that, I will create a new request from that branch. Meanwhile, marking this PR as draft, avoiding the merging

@4513 4513 marked this pull request as draft April 12, 2024 06:34
@glensc
Copy link
Contributor

glensc commented Apr 12, 2024

if you're able to force push those changes to your main branch (this branch source branch) then you don't need to make new pr. if you're unable figure out how to make force push, just add commits to this branch and I'll squash merge (then your commit messages will be lost, will be replaced what I fill).

i prefer if we can continue in this pr, so discussions don't get spread out between multiple pr's.

@4513
Copy link
Contributor Author

4513 commented Apr 12, 2024

main branch has been force pushed and contains all the changes now

@4513 4513 marked this pull request as ready for review April 12, 2024 07:53
@glensc glensc merged commit b5051e9 into perftools:main Apr 16, 2024
@4513
Copy link
Contributor Author

4513 commented Apr 16, 2024

thank you 🙂

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

2 participants