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

improve callback validation for data fetching #4056

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

afonsolpjr
Copy link

briefly:

  • Replace complex condition with is_callable
  • Ensure private/protected methods are handled correctly, since it was not even calling class methods.

long:

The Class" Redux_WordPress_Data" in '\redux-framework\redux-core\inc\classes\class-redux-wordpress-data.php', which populate some fields, is not calling class methods/callable arrays when its type is set to 'callback' and the 'args' value is the callable array.

on '\redux-framework\redux-core\inc\classes\class-redux-wordpress-data.php' , on the switch($type) statement, line 473, we have:

case 'callback':
					if ( ! empty( $args ) && is_string( $args ) && function_exists( $args ) ) {
						$data = call_user_func( $args, $current_value );
					}

					break;

which conditions does not include callable arrays, like the ones mentioned in Example 2 in this php doc

I don't know if originally it shouldn't call methods/arrays as custom callbacks, but since it is common to use callbacks in this format, it can be useful to include this.

@kprovance
Copy link
Member

I vaguely recall this being necessary some years ago because a user somehow passed the arg as something other than a string, causing a crash. It was foolish to assume theme devs would use common sense and maybe read the docs. That was my bad and thus the workaround. I don't recall if we used is_callable before, but I'm sure it's in the git history.

I'll check into it and see just to ensure we don;t go backward.

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