51423: wp_privacy_generate_personal_data_export_file fixes type match for array_merge (PHP 8) #758
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…$arr1 of function array_merge expects array, array|bool given."
hellofromtonya
commented
Nov 24, 2020
This change retains PHP Warning message (though via JSON) to alert developer where the problem exists. Why? To ensure it does not silently pass through this code, which would make debugging harder.
hellofromtonya
force-pushed
the
fix/php8.0/51423-wp-admin/includes/privacy-tools.php
branch
from
b16f03b to
00e73a0
Compare
hellofromtonya
commented
Mar 5, 2021
hellofromtonya
force-pushed
the
fix/php8.0/51423-wp-admin/includes/privacy-tools.php
branch
from
fae0f61 to
5baeff0
Compare
Contributor
Author
Testing BEFORE fixesPHP 7.3.5Results:
Error log: PHP 8.0Results:
Error log: How TestedUsed this code in a must-use to change the meta value to a string value: <?php
add_action( 'wp_privacy_personal_data_export_file', function( $request_id ) {
update_post_meta( $request_id, '_export_data_grouped', 'invalid type' );
} , 5 );Tests to reproduce:
|
hellofromtonya
commented
Mar 19, 2021
added 2 commits
March 19, 2021 13:06
Why? Type casting a non-array value creates an invalid groups structure. In < PHP 8, the invalid structure is ignored. In PHP 8, it causes a fatal error: TypeError: Cannot access offset of type string on string This commit fixes this problem. When a value exists in the post meta but it's not an array, it uses the about group only, skipping the type cast and array merge.
The timestamp in the about group can be off by 1 second between the code and test. Why? It depends upon the system clock of when each current_time() is called. To avoid failed tests when this happens, this commit extracts and uses the timestamp from the report's JSON.
Contributor
Author
Testing AFTER fixesPHP 7.3.5Results:
Error log: In the downloaded zip file:
{"Personal Data Export for [email protected]":null}
<!DOCTYPE html>
<html>
<head>
<meta http-equiv='Content-Type' content='text/html; charset=UTF-8' />
<style type='text/css'>body { color: black; font-family: Arial, sans-serif; font-size: 11pt; margin: 15px auto; width: 860px; }table { background: #f0f0f0; border: 1px solid #ddd; margin-bottom: 20px; width: 100%; }th { padding: 5px; text-align: left; width: 20%; }td { padding: 5px; }tr:nth-child(odd) { background-color: #fafafa; }.return-to-top { text-align: right; }</style><title>Personal Data Export for [email protected]</title></head>
<body>
<h1 id="top">Personal Data Export</h1></body>
</html>PHP 8.0Results:
Error log: In the downloaded zip file:
{"Personal Data Export for [email protected]":null}
<!DOCTYPE html>
<html>
<head>
<meta http-equiv='Content-Type' content='text/html; charset=UTF-8' />
<style type='text/css'>body { color: black; font-family: Arial, sans-serif; font-size: 11pt; margin: 15px auto; width: 860px; }table { background: #f0f0f0; border: 1px solid #ddd; margin-bottom: 20px; width: 100%; }th { padding: 5px; text-align: left; width: 20%; }td { padding: 5px; }tr:nth-child(odd) { background-color: #fafafa; }.return-to-top { text-align: right; }</style><title>Personal Data Export for [email protected]</title></head>
<body>
<h1 id="top">Personal Data Export</h1></body>
</html> |
Contributor
Author
|
@jrfnl This PR is ready for a final review. Since we last did a pair programming session on it, I discovered our solution caused a breaking change. Why? When the post meta does not exist or it's value is not an array, no about group content is generated in the downloaded zip file. Why? See the updated notes in the description of this PR which includes 3v4l links to show it in action. What's different since you and I last worked on it?
|
hellofromtonya
commented
Mar 29, 2021
jrfnl
approved these changes
Mar 29, 2021
Member
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
LGTM - discussed multiple times and does what we concluded should be done.
Tests also had significant improvements, so all together a great step forward.
Contributor
Author
|
Merged via changeset https://core.trac.wordpress.org/changeset/50613. |
hellofromtonya
deleted the
fix/php8.0/51423-wp-admin/includes/privacy-tools.php
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trac ticket: https://core.trac.wordpress.org/ticket/51423
Function:
wp_privacy_generate_personal_data_export_file()Problems
When/if the
'_export_data_grouped'post meta exists but is not an array, a PHP Warning (< 8)/Error (8+) is thrown forarray_mergeargument 2 type mismatch.See in action https://3v4l.org/9gLZ8
PHP Warning/Error
Current Behavior
Case:
'_export_data_grouped'post meta is not an arrayWith these values, the following happens:
The resultant zip file looks like this:
export.jsonfile:{"Personal Data Export for [email protected]":null}index.htmlfile:Fix
Check if the post meta is an array.
array_mergeandcount_doing_it_wrong(to alert the developer where the problem exists while eliminating the warning/error)See it in action https://3v4l.org/P3VAM.
See
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.