Add constant settings support #509

joshcanhelp · 2018-07-31T19:53:34Z

Add support for defining settings in a constant. Closes #480. Replaces #488 (a review of most of this functionality was already complete there).

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:

// Storing the client secret in a constant.
// Add to wp-config.php or anywhere before WP_Auth0_Options_Generic is instantiated.
// Constant name is "AUTH0_ENV_" + option key in caps.
define( 'AUTH0_ENV_CLIENT_SECRET', 'YOUR_CLIENT_SECRET_HERE' );

The default constant name should be AUTH0_ENV_ followed by the option name to override in all caps (the prefix can be modified with the auth0_settings_constant_prefix filter). 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).

All sites in a WordPress multi-site network will use the same constant value making this an easy way to setup a network using a single Application and DB Connection.

joshcanhelp · 2018-07-31T20:24:17Z

WP_Auth0.php

@@ -150,7 +150,7 @@ public static function ready() {
 	 */
 	public static function get_tenant_region( $domain ) {
 		preg_match( '/^[\w\d\-_0-9]+\.([\w\d\-_0-9]*)[\.]*auth0\.com$/', $domain, $matches );
-		return !empty($matches[1]) ? $matches[1] : 'us';
+		return ! empty( $matches[1] ) ? $matches[1] : 'us';


joshcanhelp · 2018-07-31T20:27:20Z

phpcs-test-ruleset.xml

@@ -7,7 +7,6 @@



Code standards ruleset

lbalmaceda · 2018-08-02T15:25:12Z

lib/WP_Auth0_Options_Generic.php

+	 */
+	public function get_constant_val( $key ) {
+		$constant_name = $this->get_constant_name( $key );
+		return defined( $constant_name ) ? constant( $constant_name ) : null;


it sounds like this method is repeating the logic on the has_constant_val method. Do you think this can be improved? i.e. if tomorrow the logic to check whether a constant exists or not changes, you'll need to change 2 methods instead of 1.

I think the has_constant_val was added after and I never went back to refactor this one. Good catch 👍

lbalmaceda · 2018-08-02T15:29:10Z

lib/WP_Auth0_Options_Generic.php

-		if ( $should_update ) {
-			update_option( $this->_options_name, $options );
+		// No database update so process completed successfully.
+		if ( ! $should_update ) {


is this update something you'll normally run after setting each key-value? Think of the case of setting a bunch of variables. I guess only the last call to this method would have this value as true. Right? Since I guess is not going to perform well doing so after each call. If so, would it make sense to remove this parameter from this method and just expose the update_all method for the user to call it directly on demand? You'll need to weight the most common use cases to make this decision.

is this update something you'll normally run after setting each key-value?

It's run most of the time, yes.

Since I guess is not going to perform well doing so after each call.

Not excellent but we don't set much except in the initial setup process and, in that case, there are a number of HTTP requests that will be the main slow-down. The main time settings are saved is when the page is saved and, in that case, it's a single update.

If so, would it make sense to remove this parameter from this method

BC there so I'll punt that to when we refactor this whole system.

lbalmaceda · 2018-08-02T15:37:15Z

lib/admin/WP_Auth0_Admin_Generic.php

+	 * @param string $input_name - Input name for the field, used as option key.
+	 */
+	protected function render_const_notice( $input_name ) {
+		if ( $this->options->has_constant_val( $input_name ) ) {


this check is already done on some of the render_const_notice calls. Decide whether you'd like to do this here or outside, but don't check twice... it's an overkill. e.g. call

protected function render_radio_buttons( array $buttons, $id, $input_name, $curr_value ) { if ( $field_is_const = $this->options->has_constant_val( $input_name ) ) { $this->render_const_notice( $input_name ); }

lbalmaceda · 2018-08-02T15:37:44Z

templates/back-to-auth0.php

@@ -13,5 +13,5 @@
 	}
 </style>
 <div id="extra-options">
-	<a href="?"><?php printf( _e( '← Back to %s login', 'wp-auth0' ), $title ); ?></a>
+	<a href="?"><?php printf( __( '← Back to %s login', 'wp-auth0' ), $title ); ?></a>


what does this?

_e() echos the translated string, __() returns it. Was passing an echo call to something that echos as well, causing the printf() to be skipped.

lbalmaceda · 2018-08-02T15:38:58Z

tests/testConstantSettings.php

+
+		// Make sure we have the right number of overrides.
+		$opts = new WP_Auth0_Options();
+		$this->assertCount( 3, $opts->get_all_constant_keys() );


you can go further and check the actual key values

lbalmaceda · 2018-08-02T15:45:36Z

tests/testConstantSettings.php

+
+			$input = $this->getDomListFromTagName( $field_html, 'input' );
+			$this->assertTrue( $input->item( 0 )->hasAttribute( 'disabled' ) );
+			$this->assertNotFalse( strpos( $field_html, self::CONSTANT_NOTICE_TEXT ) );


assertNotFalse vs assertTrue ?

strpos() returns the position of the string as an integer (could be 0) or FALSE if it's not found. I want to check that the string is there so I want to assert that it's not false.

That said, there is an assertContains that works better here so I switched to that.

…efactor radio button render; fix CSS

lbalmaceda

🆗

joshcanhelp added the CH: Added label Jul 31, 2018

joshcanhelp added this to the v3-Next milestone Jul 31, 2018

joshcanhelp mentioned this pull request Jul 31, 2018

Add env settings support #488

Closed

joshcanhelp force-pushed the add-constant-settings-support branch from 04e7983 to 2a18d2c Compare July 31, 2018 20:22

joshcanhelp commented Jul 31, 2018

View reviewed changes

phpcs-test-ruleset.xml

@@ -7,7 +7,6 @@

Copy link

Contributor Author

joshcanhelp Jul 31, 2018

Choose a reason for hiding this comment

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

Code standards ruleset

joshcanhelp added the Ready for Review label Jul 31, 2018

joshcanhelp force-pushed the add-constant-settings-support branch 2 times, most recently from 1920b39 to 2f6afe9 Compare July 31, 2018 20:48

lbalmaceda reviewed Aug 2, 2018

View reviewed changes

joshcanhelp added 4 commits August 2, 2018 09:33

Fix code formatting in UI and column padding

054f77c

Add core settings override methods and tests

7ada019

Add constant notice; clear settings that are constants from saving; r…

6d1ca93

…efactor radio button render; fix CSS

PR feedback

b30eb43

joshcanhelp force-pushed the add-constant-settings-support branch from 2f6afe9 to b30eb43 Compare August 2, 2018 16:33

lbalmaceda approved these changes Aug 2, 2018

View reviewed changes

joshcanhelp removed the Ready for Review label Aug 2, 2018

joshcanhelp merged commit f7033c2 into dev Aug 2, 2018

joshcanhelp deleted the add-constant-settings-support branch August 2, 2018 18:19

joshcanhelp mentioned this pull request Aug 9, 2018

Optionally load client ID, secret and domain from environment #480

Closed

github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add constant settings support #509

Add constant settings support #509

joshcanhelp commented Jul 31, 2018 •

edited

Loading

joshcanhelp Jul 31, 2018

joshcanhelp Jul 31, 2018

lbalmaceda Aug 2, 2018

joshcanhelp Aug 2, 2018

lbalmaceda Aug 2, 2018

joshcanhelp Aug 2, 2018

lbalmaceda Aug 2, 2018

lbalmaceda Aug 2, 2018

joshcanhelp Aug 2, 2018

lbalmaceda Aug 2, 2018

lbalmaceda Aug 2, 2018

joshcanhelp Aug 2, 2018

lbalmaceda left a comment

Add constant settings support #509

Add constant settings support #509

Conversation

joshcanhelp commented Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment

joshcanhelp commented Jul 31, 2018 •

edited

Loading