Connection Package: add is_active method
#12544
Conversation
roccotripaldi
added
Connect Flow
This is an automated check which relies on |
| $media_keys = array_keys( $_FILES['media'] ); | ||
|
|
||
| $token = Jetpack_Data::get_access_token( get_current_user_id() ); | ||
| $token = self::init()->connection_manager->get_access_token( get_current_user_id() ); |
There was a problem hiding this comment.
This feels pretty noisy, both in terms of the self::init() part, and the length of the connection_manager prop, and the fact that the prop is accessed as a var rather than through a method.
What about something like:
self::connection()->get_access_token( get_current_user_id() );and elsewhere:
static function connection() {
// maybe memoize this?
self::init();
return new Connection_Manager();
}There was a problem hiding this comment.
Then anywhere else in the code we like, we can call Jetpack::connection() to get a connection manager instance, at least as shorthand unless the alternative is necessary.
There was a problem hiding this comment.
Alternatively, a static entry point for connections:
// in a package file, e.g. Automattic/Jetpack/Connection.php
namespace Automattic\Jetpack;
use \Automattic\Jetpack\Connection\Manager as Connection_Manager;
function Connection() {
return new Connection_Manager();
}
class Connection {
// dummy class?
}// this acts as a handy singleton in other places, and doesn't need a constructor, but
// unfortunately can't be aliased or imported, so requires a dummy class in the same file
$token = Connection()->get_access_token( $user_id );
gravityrail
left a comment
gravityrail
left a comment
There was a problem hiding this comment.
I'm going to switch status to approve as these cleanups can be made in another PR.
@roccotripaldi if you feel my suggestions are worth exploring, either backlog a ticket or open anothre PR after this one is merged.
| @@ -1,5 +1,7 @@ | |||
| <?php //phpcs:ignore | |||
|
|
|||
| use \Automattic\Jetpack\Connection\Manager as Connection_Manager; | |||
There was a problem hiding this comment.
Why the leading slash? I'd suggest removing it.
| "version": "1.0.0", | ||
| "require": {}, | ||
| "require": { | ||
| "automattic/jetpack-options": "^1.0", |
| $possible_normal_tokens[] = $stored_blog_token; | ||
| } | ||
|
|
||
| $defined_tokens = \Jetpack_Constants::is_defined( 'JETPACK_BLOG_TOKEN' ) |
There was a problem hiding this comment.
Should we be consuming Jetpack_Constants from the constants package instead?
There was a problem hiding this comment.
We are! It's class mapped!
There was a problem hiding this comment.
Nice, yeah, you're right! Yeah, seems like we need to land #12558 to make this more obvious 😉
| @@ -1,5 +1,7 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Are we also planning to fix the tests before landing this one?
|
idk - this was a bad idea. will try an other approach |
The Connection_Manager is_active will replace
Jetpack::is_activeI'm not yet going through the code base to remove usage, but instead throwing a deprecated notice.
is_activewas dependent onJetpack_Data::get_access_token()I've also deprectated that method and started removing usages.
There remains some to remove, I'd ideally like to remove
class.jetpack-data.phpaltogether.To test:
Ensure you can still connect / disconnect a site.