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

Private fields #101

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Private fields #101

wants to merge 5 commits into from

Conversation

Camwyn
Copy link
Contributor

@Camwyn Camwyn commented Nov 7, 2023

Don't send fields that are marked private!

@Camwyn Camwyn requested a review from borkweb November 7, 2023 16:28
@Camwyn Camwyn self-assigned this Nov 7, 2023
@Camwyn
Copy link
Contributor Author

Camwyn commented Nov 7, 2023

Context on test fatal: WordPress/WordPress-Coding-Standards#2219

To correct/prevent the fatal, I've updated both automattic/vipwpcs and wp-coding-standards/wpcs

@Camwyn Camwyn requested a review from tarecord November 7, 2023 17:48
@Camwyn Camwyn requested review from bordoni and lucatume November 13, 2023 15:02
*/
public function clean_private_data( $data ): array {
foreach ( $data as &$details ) {
// remove private info.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// remove private info.
// Remove private info.

*
* @return array<string,mixed> Filtered Site Health data.
*/
public function clean_private_data( $data ): array {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're sure the $data parameter will always be an array, then type-hint it.
If you cannot be always sure of that, add an is_array($data) check to bail if $data is not an array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same applies to the return type: either you're sure of the type or not.
If you're not, remove the return type.

Comment on lines +70 to +75
$details['fields'] = array_filter(
$details['fields'],
function ( $field ) {
return empty( $field['private'] );
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

When the data is removed, the array_filter function will leave "holes" in the numeric indexes.
Some code down the line might assume continuity in the indexes.
Use array_values to restore continuity:

Suggested change
$details['fields'] = array_filter(
$details['fields'],
function ( $field ) {
return empty( $field['private'] );
}
);
$details['fields'] = array_values( array_filter(
$details['fields'],
function ( $field ) {
return empty( $field['private'] );
}
) );

foreach ( $data as &$details ) {
// remove private info.
$details['fields'] = array_filter(
$details['fields'],
Copy link
Contributor

Choose a reason for hiding this comment

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

You're assuming $details['fields'] will be set, check with isset first.

Copy link
Contributor

@tarecord tarecord left a comment

Choose a reason for hiding this comment

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

Picking this back up. Everything looks good to me once @lucatume's suggestions are made and the conflicts are resolved.

Nice work @Camwyn! 🌮

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.

3 participants