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

Remove variable that is globally accessible #2663 #2690

Merged

Conversation

raftaar1191
Copy link
Contributor

Description

PR to fix #2663

How Has This Been Tested?

Manually Tested

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@raftaar1191 raftaar1191 changed the base branch from release/2.0.1 to release/2.0.2 January 22, 2018 21:25
@raftaar1191
Copy link
Contributor Author

@DevinWalker @ravinderk

Need to Create a blog post and inform all the developer about this ( That we are removing the second parameter from the do_action of give_checkout_error_checks as this is accessible from the $_POST directly )

@kevinwhoffman
Copy link
Contributor

We should not remove the parameter altogether since it would break functionality for any developer relying on it. We should instead follow WordPress core's lead by renaming the deprecated parameter and noting it in the DocBlock.

/**
 * Fires after validating donation form fields.
 *
 * Allow you to hook to donation form errors.
 *
 * @since 1.0
 * @since 2.0.2 Second parameter deprecated in favor of $_POST.
 *
 * @param bool|array $valid_data Validated fields.
 * @param array      $deprecated Deprecated. Use $_POST instead.
 */
do_action( 'give_checkout_error_checks', $valid_data, $deprecated );

Copy link
Member

@DevinWalker DevinWalker left a comment

Choose a reason for hiding this comment

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

Let's deprecate this in core like @kevinwhoffman describes in the comments rather than remove it entirely. I see that breaking folks who don't update add-ons.

@raftaar1191
Copy link
Contributor Author

Thanks, @kevinwhoffman ya I love the example.

@DevinWalker @kevinwhoffman
Just one question what I supposed to passed in $deprecated should I pass NULL to it

do_action( 'give_checkout_error_checks', $valid_data, $deprecated = NULL );

@DevinWalker
Copy link
Member

@raftaar1191 no don't pass null that would basically be the same thing as removing it. Preserve the passed value so it doesn't break things.

@raftaar1191
Copy link
Contributor Author

Thanks @DevinWalker for you reply updated the PR

@kevinwhoffman
Copy link
Contributor

Good catch on keeping $deprecated = $_POST so that data is still available within any hooked functions.

@DevinWalker DevinWalker merged commit 08afc23 into impress-org:release/2.0.2 Jan 25, 2018
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.

None yet

3 participants