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.

Sign up for GitHub

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

Jump to bottom

Add sniff to delete_option() + add_option #637

Open
GaryJones opened this issue Mar 11, 2021 · 4 comments · May be fixed by #648
Open

Add sniff to delete_option() + add_option #637

GaryJones opened this issue Mar 11, 2021 · 4 comments · May be fixed by #648
Labels
Type: Enhancement
Milestone
3.x

Comments

@GaryJones
Copy link
Contributor

What problem would the enhancement address for VIP?

As explained at woocommerce/woocommerce#27696 there is a race condition when delete_option('foo') is followed by add_option('foo', '...').

Describe the solution you'd like

Having a sniff that looked for a delete_option() call followed by an add_option() immediately
or at some point within the same scope, for the same option key, could flag this race condition and suggest using update_option() instead.

What code should be reported as a violation?

function reset_settings() {
	$defaults = get_default_settings();
	delete_option( 'my_settings' );
	add_option( 'my_settings', $defaults );
}

Likely to need some consideration of an option key that is using a variable.

What code should not be reported as a violation?

  • add_option() in a different scope.
  • different option key (including different variable).
@rebeccahum
Copy link
Contributor

or at some point within the same scope

Do you have any examples on what would be considered in scope?

@GaryJones
Copy link
Contributor Author

Within the same function or method call.

@rebeccahum
Copy link
Contributor

rebeccahum commented Mar 29, 2021 •
edited
Loading

Starting to work on this one roughly...
I believe that the add_option() call with the same option name would have to immediately follow the delete_option(). Being in the same function or method call may be too broad of a scope? (I'm not sure.)

E.g. the sniff would not flag this as a race condition:

function reset_settings() {
	$defaults = get_default_settings();
	delete_option( 'my_settings' );

	// a whole bunch of other stuff is done here before the option gets added back

	add_option( 'my_settings', $defaults );
}

Or this one:

function reset_settings() {
	$defaults = get_default_settings();
	delete_option( 'my_settings' );
	add_option( 'settings', [] );
	add_option( 'my_settings', $defaults );
}

@rebeccahum rebeccahum linked a pull request Mar 31, 2021 that will close this issue
Open
@GaryJones
Copy link
Contributor Author

Can certainly start with looking at the calls being in consecutive statements.

@rebeccahum rebeccahum mentioned this issue Aug 16, 2021
Merged
6 tasks
@GaryJones GaryJones added this to the 3.x milestone Aug 26, 2023
@rebeccahum rebeccahum modified the milestones: 3.0.1, 3.x May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Type: Enhancement
Projects
None yet
Milestone
3.x
Development

Successfully merging a pull request may close this issue.

Add new OptionsRaceConditionSniff Automattic/VIP-Coding-Standards
2 participants
@GaryJones @rebeccahum