-
Notifications
You must be signed in to change notification settings - Fork 100
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
Add Speculation Rules settings and UI #939
Add Speculation Rules settings and UI #939
Conversation
…or defensive coding, mostly relevant for tests where setting is not registered).
'enum' => array_keys( plsr_get_mode_labels() ), | ||
), | ||
'eagerness' => array( | ||
'description' => __( 'Whether to trigger on click (conservative), on hover (moderate), or on even the slight suggestion (eager) that the user may navigate to the URL.', 'performance-lab' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if any change is made above it should be reflected here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good way for me to stay in the loop on this? If not, it would be great if you could ping me should there be related discussions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an internal mailing list where Chrome stuff spanning this area is discussed, and sometimes that includes status updates (though it also has other discussion that may be noise to you). I'm in the process of refreshing an evergreen status doc, too. I'll try to remember to send it to you when done.
Co-authored-by: Weston Ruter <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
$option = get_option( 'plsr_speculation_rules' ); | ||
if ( ! $option ) { | ||
$option = plsr_get_setting_default(); | ||
} | ||
|
||
$mode = $option['mode']; | ||
$eagerness = $option['eagerness']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While unlikely, couldn't it be that perhaps $option['mode']
isn't set here? If someone put an array into the plsr_speculation_rules
without setting that key, then either $option['mode']
or $option['eagerness']
may not be set. So for extra defensive coding, I think this would be even more robust:
$option = get_option( 'plsr_speculation_rules' ); | |
if ( ! $option ) { | |
$option = plsr_get_setting_default(); | |
} | |
$mode = $option['mode']; | |
$eagerness = $option['eagerness']; | |
$option = get_option( 'plsr_speculation_rules' ); | |
if ( ! is_array( $option ) ) { | |
$option = array(); | |
} | |
$option = array_merge( plsr_get_setting_default(), $option ); | |
$mode = $option['mode']; | |
$eagerness = $option['eagerness']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is extremely unlikely as in the scenario you're describing the value would still always be sanitized at the lowest level (within update_option()
) to include both values, per the setting registration.
Still, fair enough to do this from a defensive coding perspective. Updated in d490f93
foreach ( $fields as $slug => $args ) { | ||
add_settings_field( | ||
"plsr_speculation_rules_{$slug}", | ||
$args['title'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if my suggestion here is warranted as I don't recall how this argument is even used versus what is passed to plsr_render_settings_field()
$args['title'], | |
esc_html( $args['title'] ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's needed, as the output of this is handled by core and thus responsibility of core. This isn't something WPCS complains about, so I don't think it's needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic!
Summary
Fixes #906
Relevant technical choices
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.