-
Notifications
You must be signed in to change notification settings - Fork 96
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 env settings support #488
Conversation
lib/WP_Auth0_Options.php
Outdated
@@ -23,14 +37,6 @@ public function set_connection( $key, $value ) { | |||
$this->set( 'connections', $options['connections'] ); | |||
} | |||
|
|||
public function get_connection( $key, $default = null ) { |
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.
Moved to parent, WP_Auth0_Options_Generic
b80735d
to
077bb49
Compare
@@ -0,0 +1,152 @@ | |||
#!/usr/bin/env bash |
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.
All boilerplate from https://developer.wordpress.org/cli/commands/scaffold/plugin-tests/
@@ -0,0 +1,33 @@ | |||
<?php |
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.
Boilerplate from https://developer.wordpress.org/cli/commands/scaffold/plugin-tests/
lib/admin/WP_Auth0_Admin.php
Outdated
foreach ( $this->sections as $name => $section ) { | ||
$input = $section->input_validator( $input, $old_options ); |
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.
Handles $old_options
the same internally.
lib/admin/WP_Auth0_Admin.php
Outdated
|
||
$old_options = $this->a0_options->get_options(); | ||
|
||
$input['connections'] = $old_options['connections']; |
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 was stopping certain options from being saved.
lib/WP_Auth0_Options_Generic.php
Outdated
|
||
if ( $should_update ) { | ||
update_option( $this->_options_name, $options ); | ||
if ( null === $this->get_constant_val( $key ) ) { |
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.
Do not save if we have a constant value
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.
why not do the opposite check and return faster?
if {! defined as constant){
return;
}
//continue
avoids shifting all the remaining code to the right and IMO helps readability
* | ||
* @link https://auth0.com/docs/cms/wordpress/extending#wp_auth0_get_option | ||
*/ | ||
public function get_connection( $key, $default = null ) { |
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.
Moved from lib/WP_Auth0_Options.php
and simplified
@@ -0,0 +1,15 @@ | |||
<?xml version="1.0"?> |
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.
Boilerplate from https://developer.wordpress.org/cli/commands/scaffold/plugin-tests/
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.
As a general comment, it's missing a small doc (in readme why not? or https://auth0.com/docs/cms/wordpress ) about how to set up the plugin using constants. I know you have a line on the wp-dashboard for each overridable input, but might be good to see a mention of the feature somewhere else.
.gitignore
Outdated
@@ -8,3 +8,4 @@ wp-phptidy.php | |||
/account_cleanup/vendor/ | |||
/vendor/ | |||
/rules/ | |||
composer.lock |
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.
are you sure this file should be ignored? AFAIK, lock files are defining specific dependencies' versions this project runs fine with and should always be committed.
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.
It can go either way but, in thinking about it, we test this thoroughly with a specific combination and this is closer to an application than a library ... probably best to keep. Will add back!
lib/WP_Auth0_Options_Generic.php
Outdated
foreach ( $option_keys as $key ) { | ||
$const_name = $this->get_constant_name( $key ); | ||
if ( defined( $const_name ) ) { | ||
$this->constant_opts[$key] = constant( $const_name ); |
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.
why do you need to call constant ( {value} )
and not use it directly?
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.
Because I'm using a dynamic constant name. If I did $this->constant_opts[$key] = $const_name
then the value would be the constant name.
lib/WP_Auth0_Options_Generic.php
Outdated
// Check for constant overrides and replace. | ||
if ( ! empty( $this->constant_opts ) ) { | ||
$options = array_merge( $options, $this->constant_opts ); | ||
} | ||
|
||
$this->_opt = $options; |
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.
once this is set I guess the user can't override a constant value on runtime, can they?
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.
Nope, compile-time only. That's also way I bail on the set
method if there is a constant already present.
lib/WP_Auth0_Options_Generic.php
Outdated
* @return array | ||
*/ | ||
public function get_all_constant_keys() { | ||
return ! empty( $this->constant_opts ) ? array_keys( $this->constant_opts ) : array(); |
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.
$constant_opts
is already an empty array (default value at the top of this class). Calling array_keys
passing an empty array returns an empty array. No need the IF here. Should be: array_keys( $this->constant_opts )
.
lib/WP_Auth0_Options_Generic.php
Outdated
|
||
if ( $should_update ) { | ||
update_option( $this->_options_name, $options ); | ||
if ( null === $this->get_constant_val( $key ) ) { |
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.
why not do the opposite check and return faster?
if {! defined as constant){
return;
}
//continue
avoids shifting all the remaining code to the right and IMO helps readability
lib/admin/WP_Auth0_Admin_Generic.php
Outdated
esc_attr( $this->_option_name ), | ||
esc_attr( $input_name ), | ||
esc_attr( $id ), | ||
$this->_textarea_rows, | ||
$field_is_const ? ' disabled' : '', |
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.
hi mom
lib/admin/WP_Auth0_Admin_Generic.php
Outdated
esc_attr( $id ), | ||
esc_attr( $this->_option_name ), | ||
esc_attr( $input_name ), | ||
esc_attr( $id ), | ||
esc_attr( $value ), | ||
checked( $selected, true, false ), | ||
$disabled ? ' disabled' : '', |
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'm on gh
tests/testWPAuth0Options.php
Outdated
public function testThatConstructorStoresConstants() | ||
{ | ||
// Set a few constant overrides. | ||
define( 'AUTH0_ENV_DOMAIN', rand() ); |
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 this is a test, rand()
is setting an invalid value for this constant. I thought these were being validated, or is that run only after the user changes something in the wp dashboard?
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.
Just validated to be not empty at this point. Definitely should have better validation (I have a task already)
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.
YEAH. (Sorry caps lock on). In fact, tests values should look (and be validated!) like the actual expected values. No rush on this, but good to take note 👍
* | ||
* @return string|null | ||
*/ | ||
public function get_constant_val( $key ) { |
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 think this be transparent. What about making this method private and having a single "source" of values? The call to get ($key)
would return either the constant defined value or the one from options. Found coming back from the tests that were asserting that both get
and get_constant_val
returned the same value. https://github.com/auth0/wp-auth0/pull/488/files#diff-25c8182a46a822c28131501910ea29faR73
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.
get_constant_val()
is used in another class. I also don't want to pollute getter methods with that logic, I'd like to keep it in get_options()
(there are 2 getters based on the settings type, both use the override)
tests/testWPAuth0Options.php
Outdated
/** | ||
* ≤ | ||
*/ | ||
public function testThatFiltersOverrideValues () { |
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 wp_auth0_get_option
a random name you decided to use for this test or why is it hardcoded?
I don't follow what's happening in this test. Can you please explain it ?
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.
wp_auth0_get_option
is a filter, hard-coded machine name. Checks that this happens:
https://auth0.com/docs/cms/wordpress/extending#wp_auth0_get_option
@lbalmaceda - RE: the documentation ... it's not there because the feature is not active until this PR is merged and the plugin is released :) |
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.
LG 👨🎓
Pending bug resolution. Discussing internally.
57a9c1f
to
27ffbe1
Compare
@@ -16,21 +16,6 @@ public function is_wp_registration_enabled() { | |||
return is_multisite() ? users_can_register_signup_filter() : get_site_option( 'users_can_register' ); | |||
} | |||
|
|||
public function set_connection( $key, $value ) { |
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.
Moved to super class WP_Auth0_Options_Generic
$this->set( 'connections', $options['connections'] ); | ||
} | ||
|
||
public function get_connection( $key, $default = null ) { |
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.
Moved to super class WP_Auth0_Options_Generic
@@ -26,7 +26,7 @@ jQuery(document).ready(function ($) { | |||
} | |||
|
|||
// Look for additional fields to display | |||
if ( wpAuth0LockGlobalFields ) { | |||
if ( typeof wpAuth0LockGlobalFields !== 'undefined' ) { |
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.
Sneaking this in ... found a JS error from #491
27ffbe1
to
90da155
Compare
Re-done in #509 |
Add support for defining settings in a constant. Closes #480.
This will allow for sensitive data like the client secret, API token, and migration token to be stored in constants. The options class now looks for constants of a specific name being defined, like so:
The constant name should be
AUTH0_ENV_
followed by the option name to override in all caps. Option keys listed here can all be overridden.Note: the
migration_token
value is generated by the plugin when user migration is turned on. If there is already a value in the admin, make sure to set the constant to the same value. If that value needs to change, it also must be changed in the user migration script for the database connection being used.Once that is set,
WP_Auth0_Options_Generic::get_options()
will look for values before loading all options.WP_Auth0_Options_Generic::get()
will pull from this adjusted array.The settings field will change display based on this new value and show the constant being used:
Saving the settings page after setting a constant value will validate the constant-set values and delete them from the options array being saved to the database (important to mention that in upcoming docs).