refactor: 🔥 Simplify the PayPalBearerFactory

We do not need to know or override the current
ConnectionState, since the presence/value of the
client_id and client_secret define the state

Author: stracker-phil

modules/ppcp-api-client/services.php

@@ -179,7 +179,6 @@
 	 */
 	'api.factory.paypal-bearer'                      => static function ( ContainerInterface $container ): PayPalBearerFactory {
 		return new PayPalBearerFactory(
-			$container->get( 'settings.connection-state' ),
 			$container->get( 'api.token-rate-limiter' ),
 			$container->get( 'woocommerce.logger.woocommerce' )
 		);

modules/ppcp-api-client/src/Factory/PartnersEndpointFactory.php

@@ -96,9 +96,7 @@ public function create(
 	): PartnersEndpoint {
 		$host = (string) $this->paypal_host->get_value( $is_sandbox );
 
-		// The partners endpoint always needs a full API bearer for the given
-		// credentials, independent of the ambient connection state.
-		$bearer = $this->bearer_factory->create( $host, $client_id, $client_secret, true );
+		$bearer = $this->bearer_factory->create( $host, $client_id, $client_secret );
 
 		return new PartnersEndpoint(
 			$host,

modules/ppcp-api-client/src/Factory/PayPalBearerFactory.php

@@ -1,6 +1,6 @@
 <?php
 /**
- * Builds the API bearer for a set of credentials, honoring the connection state.
+ * Builds the API bearer for a set of credentials.
  *
  * @package WooCommerce\PayPalCommerce\ApiClient\Factory
  */
@@ -17,66 +17,54 @@
 use WooCommerce\PayPalCommerce\ApiClient\Helper\Cache;
 use WooCommerce\PayPalCommerce\ApiClient\Helper\InMemoryCache;
 use WooCommerce\PayPalCommerce\Settings\Data\SettingsProvider;
-use WooCommerce\PayPalCommerce\WcGateway\Helper\ConnectionState;
 
 /**
- * Produces the bearer appropriate for the connection state, authenticated with the
- * passed client credentials.
+ * Produces the bearer that matches the supplied credentials.
  *
- * While no merchant is connected the store can only use a login-scoped
- * ConnectBearer; once connected it uses a full API PayPalBearer. This factory owns
- * that decision by consulting the shared ConnectionState, so callers do not have to.
+ * Credential presence is the connection signal: with both a client ID and secret
+ * the factory builds an authenticated PayPalBearer, otherwise it returns a
+ * login-only ConnectBearer (nothing is connected, so there is nothing to
+ * authenticate with). This mirrors PayPalBearer itself, which cannot fetch a token
+ * without credentials.
  *
  * By default the PayPalBearer gets an isolated in-memory token cache and static
  * credentials, scoping its token to the passed credentials so it never mixes with
  * another account's cached token. Pass a persistent cache and a settings provider
- * to reproduce the shared, connection-bound bearer instead.
+ * to reproduce the shared, connection-bound bearer instead; when the settings
+ * provider resolves credentials dynamically, still pass the current client ID and
+ * secret so the bearer-type decision stays correct.
  */
 class PayPalBearerFactory {
 
-	private ConnectionState $connection_state;
-
 	private TokenRateLimiter $rate_limiter;
 
 	private LoggerInterface $logger;
 
-	public function __construct(
-		ConnectionState $connection_state,
-		TokenRateLimiter $rate_limiter,
-		LoggerInterface $logger
-	) {
-		$this->connection_state = $connection_state;
-		$this->rate_limiter     = $rate_limiter;
-		$this->logger           = $logger;
+	public function __construct( TokenRateLimiter $rate_limiter, LoggerInterface $logger ) {
+		$this->rate_limiter = $rate_limiter;
+		$this->logger       = $logger;
 	}
 
 	/**
 	 * Builds the bearer for the given credentials.
 	 *
-	 * Returns a login-only ConnectBearer while the store is not connected, otherwise
-	 * a full API PayPalBearer. The connection state is read from the shared
-	 * ConnectionState unless $is_connected overrides it. Callers that operate outside
-	 * the ambient connection (such as verifying credentials mid-connect) pass the
-	 * override to force the API bearer.
+	 * Returns an authenticated PayPalBearer when both credentials are present,
+	 * otherwise a login-only ConnectBearer.
 	 *
 	 * @param string            $host          The PayPal API host.
 	 * @param string            $client_id     The client ID.
 	 * @param string            $client_secret The client secret.
-	 * @param ?bool             $is_connected  Overrides the ambient connection state when not null.
 	 * @param ?Cache            $cache         Token cache; defaults to an isolated in-memory cache.
 	 * @param ?SettingsProvider $settings      Resolves credentials dynamically when provided.
 	 */
 	public function create(
-		string $host,
-		string $client_id,
-		string $client_secret,
-		?bool $is_connected = null,
+		string $host = '',
+		string $client_id = '',
+		string $client_secret = '',
 		?Cache $cache = null,
 		?SettingsProvider $settings = null
 	): Bearer {
-		$is_connected = $is_connected ?? $this->connection_state->is_connected();
-
-		if ( ! $is_connected ) {
+		if ( '' === $client_id || '' === $client_secret ) {
 			return new ConnectBearer();
 		}
 

modules/ppcp-settings/src/Service/AuthenticationManager.php

@@ -426,9 +426,9 @@ private function request_payee(
 	): array {
 		$host = $this->connection_host->get_value( $use_sandbox );
 
-		// Verification runs before the merchant is connected, so force the API
-		// bearer for the credentials being verified instead of reading the state.
-		$bearer = $this->bearer_factory->create( $host, $client_id, $client_secret, true );
+		// The credentials being verified are non-empty, so the factory yields an
+		// authenticated bearer even though the merchant is not connected yet.
+		$bearer = $this->bearer_factory->create( $host, $client_id, $client_secret );
 
 		$orders = new Orders(
 			$host,

tests/PHPUnit/ApiClient/Factory/PartnersEndpointFactoryTest.php

@@ -49,9 +49,7 @@ private function sut(
 	 * GIVEN merchant credentials and a per-environment host resolved from the environment config
 	 * WHEN create() is called for either the sandbox or the live environment
 	 * THEN it returns a PartnersEndpoint instance
-	 * AND it requests a full API bearer from the bearer factory, using the resolved host,
-	 *     the given credentials, and forcing the "true" override so the ambient connection
-	 *     state is never consulted
+	 * AND it delegates to the bearer factory with the resolved host and given credentials
 	 *
 	 * @dataProvider environment_provider
 	 */
@@ -67,7 +65,7 @@ public function testCreateReturnsPartnersEndpointAndForcesFullApiBearer(bool $is
 		$bearer_factory = Mockery::mock(PayPalBearerFactory::class);
 		$bearer_factory->shouldReceive('create')
 			->once()
-			->with($expected_host, 'client-id', 'client-secret', true)
+			->with($expected_host, 'client-id', 'client-secret')
 			->andReturn($bearer);
 
 		$result = $this->sut($paypal_host, $partner_id, $bearer_factory)->create(

tests/PHPUnit/ApiClient/Factory/PayPalBearerFactoryTest.php

@@ -12,7 +12,6 @@
 use WooCommerce\PayPalCommerce\ApiClient\Helper\Cache;
 use WooCommerce\PayPalCommerce\Settings\Data\SettingsProvider;
 use WooCommerce\PayPalCommerce\TestCase;
-use WooCommerce\PayPalCommerce\WcGateway\Helper\ConnectionState;
 
 class PayPalBearerFactoryTest extends TestCase
 {
@@ -28,87 +27,52 @@ public function setUp(): void
 		$this->logger = Mockery::mock(LoggerInterface::class)->shouldIgnoreMissing();
 	}
 
-	private function sut(ConnectionState $connection_state): PayPalBearerFactory
+	private function sut(): PayPalBearerFactory
 	{
 		$rate_limiter = Mockery::mock(TokenRateLimiter::class);
 
-		return new PayPalBearerFactory($connection_state, $rate_limiter, $this->logger);
+		return new PayPalBearerFactory($rate_limiter, $this->logger);
 	}
 
 	/**
-	 * GIVEN a store with no explicit connection override
-	 * WHEN create() is called and the ambient connection state resolves to a given value
-	 * THEN it returns a PayPalBearer while connected, otherwise a login-only ConnectBearer
-	 *
-	 * @dataProvider ambientConnectionData
-	 */
-	public function testCreateWithoutOverrideUsesAmbientConnectionState(bool $ambientConnected, string $expectedClass): void
-	{
-		$connection_state = Mockery::mock(ConnectionState::class);
-		$connection_state->shouldReceive('is_connected')->andReturn($ambientConnected);
-
-		$result = $this->sut($connection_state)->create('https://example.com', 'client-id', 'client-secret');
-
-		$this->assertInstanceOf($expectedClass, $result);
-	}
-
-	public function ambientConnectionData(): array
-	{
-		return [
-			'ambient connected returns PayPalBearer' => [true, PayPalBearer::class],
-			'ambient disconnected returns ConnectBearer' => [false, ConnectBearer::class],
-		];
-	}
-
-	/**
-	 * GIVEN a store whose ambient connection state is irrelevant
-	 * WHEN create() is called with an explicit connection override
-	 * THEN it returns the bearer matching the override
-	 * AND it never consults the ambient connection state
+	 * GIVEN a host and a set of client credentials
+	 * WHEN create() is called
+	 * THEN it returns a PayPalBearer only when both client ID and secret are present,
+	 * otherwise a login-only ConnectBearer
 	 *
-	 * @dataProvider overrideConnectionData
+	 * @dataProvider credentialPresenceData
 	 */
-	public function testCreateWithOverrideShortCircuitsAmbientConnectionState(bool $override, string $expectedClass): void
+	public function testCreateDecidesBearerTypeByCredentialPresence(string $clientId, string $clientSecret, string $expectedClass): void
 	{
-		$connection_state = Mockery::mock(ConnectionState::class);
-		$connection_state->shouldReceive('is_connected')->never();
-
-		$result = $this->sut($connection_state)->create(
-			'https://example.com',
-			'client-id',
-			'client-secret',
-			$override
-		);
+		$result = $this->sut()->create('https://example.com', $clientId, $clientSecret);
 
 		$this->assertInstanceOf($expectedClass, $result);
 	}
 
-	public function overrideConnectionData(): array
+	public function credentialPresenceData(): array
 	{
 		return [
-			'override true returns PayPalBearer' => [true, PayPalBearer::class],
-			'override false returns ConnectBearer' => [false, ConnectBearer::class],
+			'client id and secret present returns PayPalBearer' => ['client-id', 'client-secret', PayPalBearer::class],
+			'empty client id returns ConnectBearer' => ['', 'client-secret', ConnectBearer::class],
+			'empty client secret returns ConnectBearer' => ['client-id', '', ConnectBearer::class],
+			'no credentials returns ConnectBearer' => ['', '', ConnectBearer::class],
 		];
 	}
 
 	/**
-	 * GIVEN a store that is connected, with an explicit cache and settings provider
+	 * GIVEN non-empty client credentials, an explicit cache and settings provider
 	 * WHEN create() is called
 	 * THEN it still returns a PayPalBearer, passing both through to the constructor
 	 */
-	public function testCreateWhenConnectedWithExplicitCacheAndSettingsReturnsPayPalBearer(): void
+	public function testCreateWithExplicitCacheAndSettingsReturnsPayPalBearer(): void
 	{
-		$connection_state = Mockery::mock(ConnectionState::class);
-		$connection_state->shouldReceive('is_connected')->never();
-
 		$cache = Mockery::mock(Cache::class);
 		$settings = Mockery::mock(SettingsProvider::class);
 
-		$result = $this->sut($connection_state)->create(
+		$result = $this->sut()->create(
 			'https://example.com',
 			'client-id',
 			'client-secret',
-			true,
 			$cache,
 			$settings
 		);