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

Fixes PHP warnings on repeatable ColorPicker with an array as default #1340

Merged

Conversation

rubengc
Copy link
Contributor

@rubengc rubengc commented Feb 18, 2020

This PR fixes PHP warnings when setting up a ColorPicker field as repeatable and passing as default parameter an array caused by pass this array to preg_match() and trim() functions

Basically, this doesn't causes any error, just PHP warnings that would be awesome to silence

Check issue #1285 where everything gets explained

Fixes #1285
Fixes #1286

This PR fixes PHP warnings when setting up a ColorPicker field as repeatable and passing as default parameter an array caused by pass this array to preg_match() and trim() functions

Basically, this doesn't causes any error, just PHP warnings that would be awesome to silence

Check issue CMB2#1285 where everything gets explained

Fixes CMB2#1285
Fixes CMB2#1286
@tw2113
Copy link
Contributor

tw2113 commented Feb 18, 2020

I'd honestly prefer that the actual errors got addressed instead of just silencing them here.

@jtsternberg
Copy link
Member

I agree, I think there's a better way to ensure compatibility than telling it to shut up. :)

@rubengc
Copy link
Contributor Author

rubengc commented Feb 18, 2020

Which way do you prefer?

Checking if is an array, getting only the first value like this:

if( is_array( $meta_value ) ) {
    $meta_value = $meta_value[0]
} 

Or, only applying sanitization if is a string only:

	if( gettype( $meta_value ) === 'string' ) {
		$hex_color = '(([a-fA-F0-9]){3}){1,2}$';
		if ( preg_match( '/^' . $hex_color . '/i', $meta_value ) ) {
			// Value is just 123abc, so prepend #
			$meta_value = '#' . $meta_value;
		} elseif (
			// If value doesn't match #123abc...
			! preg_match( '/^#' . $hex_color . '/i', $meta_value )
			// And value doesn't match rgba()...
			&& 0 !== strpos( trim( $meta_value ), 'rgba' )
		) {
			// Then sanitize to just #.
			$meta_value = '#';
		}
	}

@rubengc
Copy link
Contributor Author

rubengc commented Feb 18, 2020

Ah, I forgive to mention here that this issue only happens on the empty field generated by the repeatable field

The empty field is the field used as pattern every time the field gets repeated

@jtsternberg
Copy link
Member

Best would be a new method for the sanitization, and send the meta value into the method, or do array_map to loop through the array to sanitize.

@rubengc
Copy link
Contributor Author

rubengc commented Feb 18, 2020

Ready!

@tw2113
Copy link
Contributor

tw2113 commented Feb 18, 2020

Can you resolve the conflicts coming up in the PR here?

Copy link
Member

@jtsternberg jtsternberg left a comment

Choose a reason for hiding this comment

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

Left a few suggestions. Have you tested this with a normal color picker and repeatable?

includes/types/CMB2_Type_Colorpicker.php Outdated Show resolved Hide resolved
includes/types/CMB2_Type_Colorpicker.php Outdated Show resolved Hide resolved
includes/types/CMB2_Type_Colorpicker.php Outdated Show resolved Hide resolved
includes/types/CMB2_Type_Colorpicker.php Outdated Show resolved Hide resolved
includes/types/CMB2_Type_Colorpicker.php Outdated Show resolved Hide resolved
includes/types/CMB2_Type_Colorpicker.php Outdated Show resolved Hide resolved
rubengc and others added 7 commits February 19, 2020 10:13
Co-Authored-By: Justin Sternberg <me@jtsternberg.com>
Co-Authored-By: Justin Sternberg <me@jtsternberg.com>
Co-Authored-By: Justin Sternberg <me@jtsternberg.com>
Co-Authored-By: Justin Sternberg <me@jtsternberg.com>
Co-Authored-By: Justin Sternberg <me@jtsternberg.com>
Co-Authored-By: Justin Sternberg <me@jtsternberg.com>
@rubengc
Copy link
Contributor Author

rubengc commented Feb 19, 2020

Tested with both fields and everything works perfectly

The unique issue I found is an "array to string conversion" error from the CMB2_Utils concat_attrs() function:
https://github.com/CMB2/CMB2/blob/develop/includes/CMB2_Utils.php#L584-L599

I added on this PR a change checking if is an array, turn it into a comma-separated list of values:
42bcb97

@tw2113
Copy link
Contributor

tw2113 commented Feb 19, 2020

the PR is still showing some conflicts in CMB2_Type_Colorpicker.php file here in GitHub's UI.

Copy link
Member

@jtsternberg jtsternberg left a comment

Choose a reason for hiding this comment

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

Get these changes in (and merge develop into this branch and fix merge conflict), and I think it's ready to go.

includes/CMB2_Utils.php Outdated Show resolved Hide resolved
includes/types/CMB2_Type_Colorpicker.php Outdated Show resolved Hide resolved
includes/types/CMB2_Type_Colorpicker.php Outdated Show resolved Hide resolved
rubengc and others added 3 commits August 12, 2020 17:31
Co-authored-by: Justin Sternberg <me@jtsternberg.com>
Co-authored-by: Justin Sternberg <me@jtsternberg.com>
Co-authored-by: Justin Sternberg <me@jtsternberg.com>
@rubengc
Copy link
Contributor Author

rubengc commented Oct 28, 2020

All conflicts resolved, can this get added to CMB2?

@rubengc
Copy link
Contributor Author

rubengc commented Feb 15, 2021

could this one get merged? Changes requested have been added yet

@jtsternberg jtsternberg merged commit e930d33 into CMB2:develop Feb 15, 2021
jtsternberg added a commit that referenced this pull request Feb 15, 2021
@jtsternberg
Copy link
Member

Thank you!

lipemat added a commit to lipemat/CMB2 that referenced this pull request Mar 30, 2021
* upstream/develop:
  Clean up and add props for CMB2#1413
  Sanitize URLs with HTTPS
  Add develop suffix to init class
  Add am-cli-tools
  Update changelong and version numbers and readmes, and prepare release
  Set default priority to 10 for options pages. Fixes CMB2#1410
  build field-cache key manually to remove unnecessary |'s
  Better generated array key for cached fields, fixes issue where wrong field is found. Fixes CMB2#1405
  Add to list of valid image types from get_allowed_mime_types(). Fixes CMB2#1223
  Move tab markup output to separate method, options_page_tab_nav_output. Fixes CMB2#1407
  Add cmb2_tab_group_tabs filter for adding arbitrary menu page urls to the cmb2 tabs. See CMB2#1407
  Update since tag, and add props for CMB2#1340
  Limit use of italic, including removing from field descriptions. Fixes CMB2#1404
  Add props for CMB2#1400
  move $args in deprecated_param method for 7.4
  Add develop suffix to init class
  Prepare release and changelog for 2.8.0
  Fix tests since WP_Error signature changed
  move $args in deprecated_param method for 7.4
  Use the already-existing get_priority method. Re CMB2#1380 and CMB2#1398
  Use existing "priority" field param. Fixes CMB2#1380. Closes CMB2#1398
  Add admin_menu_hook_priority box property for options boxes. Fixes CMB2#1380. Closes CMB2#1398
  Make field_can first param required to address php 8 "Required parameter follows optional parameter". Fixes CMB2#1396
  Update includes/types/CMB2_Type_Colorpicker.php
  Update includes/types/CMB2_Type_Colorpicker.php
  Update includes/CMB2_Utils.php
  Prevent array to string conversion
  Update includes/types/CMB2_Type_Colorpicker.php
  Update includes/types/CMB2_Type_Colorpicker.php
  Update includes/types/CMB2_Type_Colorpicker.php
  Update includes/types/CMB2_Type_Colorpicker.php
  Update includes/types/CMB2_Type_Colorpicker.php
  Update includes/types/CMB2_Type_Colorpicker.php
  Added sanitize_color() function and remove PHP warnings suppresions
  Fixes PHP warnings on repeatable ColorPicker with an array as default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants