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

PHP 8.1 compatibility: replace FILTER_SANITIZE_STRING #428

Merged
merged 2 commits into from
Oct 7, 2022
Merged

PHP 8.1 compatibility: replace FILTER_SANITIZE_STRING #428

merged 2 commits into from
Oct 7, 2022

Conversation

sjinks
Copy link
Contributor

@sjinks sjinks commented Jan 24, 2022

PHP 8.1 deprecated FILTER_SANITIZE_STRING. This PR replaces it with other alternatives.

Ref: Automattic/vip-go-mu-plugins#2667

class-two-factor-core.php Outdated Show resolved Hide resolved
@sjinks sjinks marked this pull request as ready for review January 24, 2022 16:49
@jeffpaul jeffpaul requested a review from kasparsd January 24, 2022 18:00
@jeffpaul jeffpaul added this to the 0.8.0 milestone Mar 11, 2022
@jeffpaul
Copy link
Member

Per yesterday's bug scrub, @georgestephanis feels the sanitize string is reasonable and could be good to merge, but is not familiar with filter_callback and not positive if core code style still recommends using array( ) over [ ] shorthand.

@jeffpaul jeffpaul modified the milestones: 0.8.0, Future Release Mar 24, 2022
Copy link
Member

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

not familiar with filter_callback

FILTER_CALLBACK is documented here, but essentially is a way to ignore the built-in FILTER_* constants, and set a callback function instead.

not positive if core code style still recommends using array( ) over [ ] shorthand

WPCS still requires longhand array syntax, so that's the code suggestions I've made here.

class-two-factor-compat.php Outdated Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
@jeffpaul jeffpaul requested a review from GaryJones September 9, 2022 16:06
@kasparsd
Copy link
Collaborator

kasparsd commented Oct 7, 2022

This looks good! I'll merge this in and some linter checks to find any other PHP8 compatibility issues.

@kasparsd kasparsd merged commit 5363ca6 into WordPress:master Oct 7, 2022
@jeffpaul jeffpaul modified the milestones: Future Release, 0.8.0 Oct 7, 2022
@iandunn
Copy link
Member

iandunn commented Oct 14, 2022

WPCS still requires longhand array syntax, so that's the code suggestions I've made here.

There was a proposal to change to the short syntax, but the decision was to keep the long one.

@jeffpaul jeffpaul modified the milestones: 0.8.0, 0.7.3 Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants