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

disallow 0x0 unit size #72

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jakewrfoster
Copy link
Member

@jakewrfoster jakewrfoster commented Oct 31, 2019

The current lack of group_is_empty allows for cases where ad-layers will save an ad unit size as 0x0. This size group then becomes impossible to remove. This PR ensures that any sizes groups that have a 0x0 ad unit will not be saved.

Screen Shot 568

@mboynes / @bcampeau: For backwards-compat reasons, is there any use-case for an ad unit being 0x0?

Copy link

@kjbenk kjbenk 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 this can be simplified

'add_more_label' => __( 'Add Size', 'ad-layers' ),
'description' => __( 'Size cannot be 0x0.', 'ad-layers' ),
'group_is_empty' => function( $values ) {
if ( isset( $values['width'], $values['height'] ) ) {
Copy link

Choose a reason for hiding this comment

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

You should just be able to do

return empty( $values['width'] ) && empty( $values['height'] );

Copy link

Choose a reason for hiding this comment

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

Also, if you really need all of these checks, then it might be easier to read stacked in a single if statement

// Valid size, save the group.
return false;
} else {
return true;
Copy link

Choose a reason for hiding this comment

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

So if the values are set but one of them is not numeric, the group should be considered empty?

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 think that the numeric checks can be removed since these fields are being sanitized by absint anyway.

Copy link

@kjbenk kjbenk left a comment

Choose a reason for hiding this comment

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

⛳️

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.

2 participants