Improve WP_Auth0_Options #418
Conversation
Adding get_lock_connections and add_lock_connection for working with separated options field. reordering option defaults, remove comments, add docblocks
| public function get_client_signing_algorithm() { | ||
| $client_signing_algorithm = $this->get('client_signing_algorithm', WP_Auth0_Api_Client::DEFAULT_CLIENT_ALG); | ||
| return $client_signing_algorithm; | ||
| return $this->get( 'client_signing_algorithm', WP_Auth0_Api_Client::DEFAULT_CLIENT_ALG ); |
There was a problem hiding this comment.
is this like this get(key, valueIfMissing)?
lib/WP_Auth0_Options.php
Outdated
| @@ -129,92 +134,124 @@ public function get_logout_url() { | |||
| return add_query_arg( 'action', 'logout', site_url( 'wp-login.php', 'login' ) ); | |||
| } | |||
|
|
|||
| /** | |||
| * Get as a string-separated string and parse to array | |||
There was a problem hiding this comment.
No. That's what the method does internally. What devs should care is "obtain the connections as an array of strings".
There was a problem hiding this comment.
"string-separated string" 😆
| * @return array | ||
| */ | ||
| public function get_lock_connections() { | ||
| $connections = $this->get( 'lock_connections' ); |
There was a problem hiding this comment.
why is the lock_connections a "string,of,connections"? It seems you are always manipulating arrays. Can it be defined as an array to avoid all this coming and going from one structure to the other?
There was a problem hiding this comment.
The field in on the settings page is just a text field so comma-separated (or line break or pipe or whatever) is about the best we can do without a UI to force multiple, separate values. This is basically to handle user input
| public function get_lock_connections() { | ||
| $connections = $this->get( 'lock_connections' ); | ||
| $connections = empty( $connections ) ? array() : explode( ',', $connections ); | ||
| return array_map( 'trim', $connections ); |
There was a problem hiding this comment.
is the trim needed because it's a user input or because you don't trust the source? (i don't know where it comes from)
lib/WP_Auth0_Options.php
Outdated
| 'amplificator_title' => '', | ||
| 'amplificator_subtitle' => '', | ||
| 'connections' => array(), | ||
| 'auth0js-cdn' => '//cdn.auth0.com/js/auth0/9.1/auth0.min.js', |
There was a problem hiding this comment.
you are using https, right?
There was a problem hiding this comment.
Some folks are, some are not ... though this URL will always be https so I guess that could be explicit here? This is just a reordering, FYI
There was a problem hiding this comment.
it's hosted by auth0 so I don't see a reason why they shouldn't be using https. They don't need to setup anything on their side. It's not like a custom domain where the certificate needs to be handled by your or something like that.. I'd enforce https on all auth0 urls. Double check with luis, but i think that's the right choice.
There was a problem hiding this comment.
this should be always https
| @@ -556,8 +556,6 @@ public function connections_validation( $old_options, $input ) { | |||
|
|
|||
| if ($input['passwordless_enabled'] && $input['passwordless_enabled'] != $old_options['passwordless_enabled']) { | |||
|
|
|||
| // $check_if_enabled = explode(',', $input['lock_connections']); | |||
There was a problem hiding this comment.
what happened to all these comments that are now removed? was it implemented somewhere?
There was a problem hiding this comment.
They've been commented for a while
There was a problem hiding this comment.
but is it something you want to do someday? are you tracking it somewhere else or are they just gone for good? 😂
There was a problem hiding this comment.
I'm not sure what it's for, it's obviously not doing anything, and it's related to the settings I'm working with so 👋
:)
lib/WP_Auth0_Options.php
Outdated
| * | ||
| * @param string $add_conn - connection name to add | ||
| */ | ||
| public function add_lock_connection( $add_conn ) { |
There was a problem hiding this comment.
I'd rename this parameter to "connection" or "conn" since the method name already describes what you are doing with it.
lib/WP_Auth0_Options.php
Outdated
| $connections = $this->get_lock_connections(); | ||
|
|
||
| // Add if it doesn't exist already | ||
| if ( ! array_key_exists( $add_conn, $connections ) ) { |
There was a problem hiding this comment.
This is a suggestion:
It could be more performant if you FIRST check that the string from $this->get( 'lock_connections' ); "contains" the trimmed $add_conn parameter.
- If it DOES contain it, return (skip). This way, you avoid converting it to an array in the first place.
- if not, then you can call the
get_lock_connections()method that prepares the array for you.
There was a problem hiding this comment.
You can't really do that can you? What if I want to add the connection google-oauth so I search for the string google in twitter,facebook,google-oauth2 and it's there so I bail out?
Adding get_lock_connections and add_lock_connection for working with
separated options field. reordering option defaults, remove comments,
add docblocks