From 4bf1d005f71b3b9db88f520f106155582122538c Mon Sep 17 00:00:00 2001 From: Giuseppe Mazzapica Date: Fri, 24 May 2024 12:20:06 +0200 Subject: [PATCH 1/4] Fix status check in PackageProxyContainer When status is STATUS_INITIALIZED or above, it is possible to access the package container. Right now, PackageProxyContainer only access the container after booting which is wrong and make usage of build() "dangerous". --- src/Container/PackageProxyContainer.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Container/PackageProxyContainer.php b/src/Container/PackageProxyContainer.php index d5e3954..207985f 100644 --- a/src/Container/PackageProxyContainer.php +++ b/src/Container/PackageProxyContainer.php @@ -66,7 +66,9 @@ private function tryContainer(): bool /** TODO: We need a better way to deal with status checking besides equality */ if ( - $this->package->statusIs(Package::STATUS_READY) + $this->package->statusIs(Package::STATUS_INITIALIZED) + || $this->package->statusIs(Package::STATUS_MODULES_ADDED) + || $this->package->statusIs(Package::STATUS_READY) || $this->package->statusIs(Package::STATUS_BOOTED) ) { $this->container = $this->package->container(); From f6af9bfde384fc24caba9c95e1ba85c852ab05d1 Mon Sep 17 00:00:00 2001 From: Giuseppe Mazzapica Date: Fri, 24 May 2024 12:29:03 +0200 Subject: [PATCH 2/4] Add test for acccess services from a built connected package --- tests/unit/PackageTest.php | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/tests/unit/PackageTest.php b/tests/unit/PackageTest.php index 3f810b5..bfa15f2 100644 --- a/tests/unit/PackageTest.php +++ b/tests/unit/PackageTest.php @@ -470,7 +470,7 @@ public function testStatusForMultipleModulesWhenNotDebug(): void * * @test */ - public function testPackageConnection(): void + public function testPackageConnectionWithBoot(): void { $module1 = $this->mockModule('module_1', ServiceModule::class); $module1->expects('services')->andReturn($this->stubServices('service_1')); @@ -497,6 +497,38 @@ public function testPackageConnection(): void static::assertInstanceOf(\ArrayObject::class, $package2->container()->get('service_1')); } + /** + * Test we can connect services across packages. + * + * @test + */ + public function testPackageConnectionWithBuildViaPackageProxyContainer(): void + { + $module1 = $this->mockModule('module_1', ServiceModule::class); + $module1->expects('services')->andReturn($this->stubServices('service_1')); + $package1 = Package::new($this->mockProperties('package_1', false)) + ->addModule($module1); + + $module2 = $this->mockModule('module_2', ServiceModule::class); + $module2->expects('services')->andReturn($this->stubServices('service_2')); + $package2 = Package::new($this->mockProperties('package_2', false)) + ->addModule($module2); + + Monkey\Actions\expectDone($package2->hookName(Package::ACTION_PACKAGE_CONNECTED)) + ->once() + ->with($package1->name(), Package::STATUS_IDLE, true); + + // Package1 is idle, so connection will use PackageProxyContainer + $connected = $package2->connect($package1); + $package1->build(); + $package2->build(); + + static::assertTrue($connected); + static::assertSame(['package_1' => true], $package2->connectedPackages()); + // retrieve a Package 1's service from Package 2's container. + static::assertInstanceOf(\ArrayObject::class, $package2->container()->get('service_1')); + } + /** * Test we can not connect services when the package how call connect is booted. * From 9e09e9f657b397128c2f037aa760f93c048f8882 Mon Sep 17 00:00:00 2001 From: Giuseppe Mazzapica Date: Fri, 24 May 2024 12:37:30 +0200 Subject: [PATCH 3/4] Update package connection docs --- docs/Package.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/Package.md b/docs/Package.md index b25fdef..9fc7bd1 100644 --- a/docs/Package.md +++ b/docs/Package.md @@ -235,8 +235,8 @@ $theme->boot(); To note: -- `Package::connect()` must be called **before** boot. If called later, no connections happen and it returns `false` -- The package to be connected might be already booted or not. In the second case the connection will happen, but before accessing its services it has to be booted, or an exception will happen. +- `Package::connect()` must be called **before** the package enters the "initialized" status, that is, before calling `Package::boot()` or `Package::build()`. If called later, no connections happen and it returns `false` +- The package to be connected might be already booted or not. In the second case the connection will happen, but before accessing its services it has to be at least built, or an exception will happen. Package connection is a great way to create reusable libraries and services that can be used by many plugins. For example, it might be possible to have a *library* that has something like this: From 5c6b99029f5ffcb0d9fdf2b6ad2f0e77f1d56245 Mon Sep 17 00:00:00 2001 From: Giuseppe Mazzapica Date: Mon, 27 May 2024 18:54:01 +0200 Subject: [PATCH 4/4] Improve tests around service connection And fail a couple of edge-cases bugs discovered via new tests --- src/Package.php | 12 +- tests/unit/PackageTest.php | 267 +++++++++++++++---------------------- 2 files changed, 113 insertions(+), 166 deletions(-) diff --git a/src/Package.php b/src/Package.php index 49bf96a..91b7654 100644 --- a/src/Package.php +++ b/src/Package.php @@ -275,9 +275,10 @@ public function connect(Package $package): bool // Don't connect, if already booted or boot failed $failed = $this->statusIs(self::STATUS_FAILED); - if ($failed || $this->statusIs(self::STATUS_BOOTED)) { - $status = $failed ? 'errored' : 'booted'; - $error = "{$errorMessage} to a {$status} package."; + if ($failed || $this->checkStatus(self::STATUS_INITIALIZED, '>=')) { + $reason = $failed ? 'an errored package' : 'a package with a built container'; + $status = $failed ? 'failed' : 'built_container'; + $error = "{$errorMessage} to {$reason}."; do_action( $this->hookName(self::ACTION_FAILED_CONNECTION), $packageName, @@ -315,7 +316,10 @@ static function () use ($package): Properties { return true; } catch (\Throwable $throwable) { - if (isset($packageName)) { + if ( + isset($packageName) + && (($this->connectedPackages[$packageName] ?? false) !== true) + ) { $this->connectedPackages[$packageName] = false; } $this->handleFailure($throwable, self::ACTION_FAILED_BUILD); diff --git a/tests/unit/PackageTest.php b/tests/unit/PackageTest.php index e9a95b0..75a9811 100644 --- a/tests/unit/PackageTest.php +++ b/tests/unit/PackageTest.php @@ -470,30 +470,24 @@ public function testStatusForMultipleModulesWhenNotDebug(): void * * @test */ - public function testPackageConnectionWithBoot(): void + public function testConnectIdlePackageFromIdlePackage(): void { - $module1 = $this->stubModule('module_1', ServiceModule::class); - $module1->expects('services')->andReturn($this->stubServices('service_1')); - $package1 = Package::new($this->stubProperties('package_1', false)) - ->addModule($module1); - - $module2 = $this->stubModule('module_2', ServiceModule::class); - $module2->expects('services')->andReturn($this->stubServices('service_2')); - $package2 = Package::new($this->stubProperties('package_2', false)) - ->addModule($module2); + $package1 = $this->stubSimplePackage('1'); + $package2 = $this->stubSimplePackage('2'); Monkey\Actions\expectDone($package2->hookName(Package::ACTION_PACKAGE_CONNECTED)) ->once() - ->with($package1->name(), Package::STATUS_IDLE, false); - - $package1->boot(); + ->with($package1->name(), Package::STATUS_IDLE, true); $connected = $package2->connect($package1); - $package2->boot(); static::assertTrue($connected); - static::assertSame(['package_1' => true], $package2->connectedPackages()); - // retrieve a Package 1's service from Package 2's container. + static::assertTrue($package2->isPackageConnected($package1->name())); + + $package1->build(); + $package2->build(); + + // Retrieve a Package 1's service from Package 2's container. static::assertInstanceOf(\ArrayObject::class, $package2->container()->get('service_1')); } @@ -502,195 +496,161 @@ public function testPackageConnectionWithBoot(): void * * @test */ - public function testPackageConnectionWithBuildViaPackageProxyContainer(): void + public function testConnectBuiltPackageFromIdlePackage(): void { - $module1 = $this->mockModule('module_1', ServiceModule::class); - $module1->expects('services')->andReturn($this->stubServices('service_1')); - $package1 = Package::new($this->mockProperties('package_1', false)) - ->addModule($module1); - - $module2 = $this->mockModule('module_2', ServiceModule::class); - $module2->expects('services')->andReturn($this->stubServices('service_2')); - $package2 = Package::new($this->mockProperties('package_2', false)) - ->addModule($module2); + $package1 = $this->stubSimplePackage('1'); + $package2 = $this->stubSimplePackage('2'); Monkey\Actions\expectDone($package2->hookName(Package::ACTION_PACKAGE_CONNECTED)) ->once() ->with($package1->name(), Package::STATUS_IDLE, true); - // Package1 is idle, so connection will use PackageProxyContainer - $connected = $package2->connect($package1); $package1->build(); - $package2->build(); + + $connected = $package2->connect($package1); static::assertTrue($connected); - static::assertSame(['package_1' => true], $package2->connectedPackages()); - // retrieve a Package 1's service from Package 2's container. + static::assertTrue($package2->isPackageConnected($package1->name())); + + $package2->build(); + + // Retrieve a Package 1's service from Package 2's container. static::assertInstanceOf(\ArrayObject::class, $package2->container()->get('service_1')); } /** - * Test we can not connect services when the package how call connect is booted. - * * @test */ - public function testPackageConnectionFailsIfBootedWithDebugOff(): void + public function testConnectBootedPackageFromIdlePackage(): void { - $module1 = $this->stubModule('module_1', ServiceModule::class); - $module1->expects('services')->andReturn($this->stubServices('service_1')); - $package1 = Package::new($this->stubProperties('package_1', false)) - ->addModule($module1); + $package1 = $this->stubSimplePackage('1'); + $package2 = $this->stubSimplePackage('2'); - $module2 = $this->stubModule('module_2', ServiceModule::class); - $module2->expects('services')->andReturn($this->stubServices('service_2')); - $package2 = Package::new($this->stubProperties('package_2', false)) - ->addModule($module2); + Monkey\Actions\expectDone($package2->hookName(Package::ACTION_PACKAGE_CONNECTED)) + ->once() + ->with($package1->name(), Package::STATUS_IDLE, false); $package1->boot(); - $package2->boot(); - Monkey\Actions\expectDone($package2->hookName(Package::ACTION_FAILED_CONNECTION)) - ->once() - ->with($package1->name(), \Mockery::type(\WP_Error::class)); + $connected = $package2->connect($package1); - Monkey\Actions\expectDone($package2->hookName(Package::ACTION_FAILED_BUILD)) - ->once() - ->whenHappen( - function (\Throwable $throwable): void { - $this->assertThrowableMessageMatches($throwable, 'failed connect.+?booted'); - } - ); + static::assertTrue($connected); + static::assertTrue($package2->isPackageConnected($package1->name())); - $connected = $package2->connect($package1); + $package2->build(); - static::assertFalse($connected); - static::assertSame(['package_1' => false], $package2->connectedPackages()); + // Retrieve a Package 1's service from Package 2's container. + static::assertInstanceOf(\ArrayObject::class, $package2->container()->get('service_1')); } /** - * Test we can not connect services when the package how call connect is booted. - * * @test */ - public function testPackageConnectionFailsIfBootedWithDebugOn(): void + public function testConnectBuiltPackageFromBuildPackageFailsDebugOff(): void { - $module1 = $this->stubModule('module_1', ServiceModule::class); - $module1->expects('services')->andReturn($this->stubServices('service_1')); - $package1 = Package::new($this->stubProperties('package_1', true)) - ->addModule($module1); + $package1 = $this->stubSimplePackage('1'); + $package2 = $this->stubSimplePackage('2'); - $module2 = $this->stubModule('module_2', ServiceModule::class); - $module2->expects('services')->andReturn($this->stubServices('service_2')); - $package2 = Package::new($this->stubProperties('package_2', true)) - ->addModule($module2); + Monkey\Actions\expectDone($package2->hookName(Package::ACTION_FAILED_CONNECTION)) + ->once() + ->with($package1->name(), \Mockery::type(\WP_Error::class)); - $package1->boot(); - $package2->boot(); + $package1->build(); + $package2->build(); + + static::assertFalse($package2->connect($package1)); + } + + /** + * @test + */ + public function testConnectBuiltPackageFromBuildPackageFailsDebugOn(): void + { + $package1 = $this->stubSimplePackage('1'); + $package2 = $this->stubSimplePackage('2', true); Monkey\Actions\expectDone($package2->hookName(Package::ACTION_FAILED_CONNECTION)) ->once() ->with($package1->name(), \Mockery::type(\WP_Error::class)); - Monkey\Actions\expectDone($package2->hookName(Package::ACTION_FAILED_BUILD)) - ->once() - ->whenHappen( - function (\Throwable $throwable): void { - $this->assertThrowableMessageMatches($throwable, 'failed connect.+?booted'); - } - ); + $package1->build(); + $package2->build(); - $this->expectExceptionMessageMatches('/failed connect.+?booted/i'); + $this->expectExceptionMessageMatches('/built container/i'); $package2->connect($package1); } /** - * Test we can connect services even if target package is not booted yet. - * * @test */ - public function testPackageConnectionWithProxyContainer(): void + public function testConnectBuiltPackageFromBootedPackageFailsDebugOff(): void { - $module1 = $this->stubModule('module_1', ServiceModule::class); - $module1->expects('services')->andReturn($this->stubServices('service_1')); - $package1 = Package::new($this->stubProperties('package_1', false)) - ->addModule($module1); + $package1 = $this->stubSimplePackage('1'); + $package2 = $this->stubSimplePackage('2'); - $module2 = $this->stubModule('module_2', ServiceModule::class); - $module2->expects('services')->andReturn($this->stubServices('service_2')); - $package2 = Package::new($this->stubProperties('package_2', false)) - ->addModule($module2); + Monkey\Actions\expectDone($package2->hookName(Package::ACTION_FAILED_CONNECTION)) + ->once() + ->with($package1->name(), \Mockery::type(\WP_Error::class)); - $connected = $package2->connect($package1); + $package1->build(); $package2->boot(); - static::assertTrue($connected); - static::assertSame(['package_1' => true], $package2->connectedPackages()); + static::assertFalse($package2->connect($package1)); + } - // We successfully connect before booting, but we need to boot to retrieve services - $package1->boot(); - $container = $package2->container(); + /** + * @test + */ + public function testConnectBuiltPackageFromBootedPackageFailsDebugOn(): void + { + $package1 = $this->stubSimplePackage('1'); + $package2 = $this->stubSimplePackage('2', true); - // retrieve a Package 1's service from Package 2's container. - $connectedService = $container->get('service_1'); - // retrieve the Package 1's properties from Package 2's container. - $connectedProperties = $container->get('package_1.' . Package::PROPERTIES); + Monkey\Actions\expectDone($package2->hookName(Package::ACTION_FAILED_CONNECTION)) + ->once() + ->with($package1->name(), \Mockery::type(\WP_Error::class)); + + $package1->build(); + $package2->boot(); - static::assertInstanceOf(\ArrayObject::class, $connectedService); - static::assertInstanceOf(Properties::class, $connectedProperties); - static::assertSame('package_1', $connectedProperties->baseName()); + $this->expectExceptionMessageMatches('/built container/i'); + $package2->connect($package1); } /** - * Test that connecting packages not booted, fails when accessing services - * * @test */ - public function testPackageConnectionWithProxyContainerFailsIfNoBoot(): void + public function testAccessingServicesFromIdleConnectedPackageFails(): void { - $module1 = $this->stubModule('module_1', ServiceModule::class); - $module1->expects('services')->andReturn($this->stubServices('service_1')); - $package1 = Package::new($this->stubProperties('package_1', false)) - ->addModule($module1); - - $module2 = $this->stubModule('module_2', ServiceModule::class); - $module2->expects('services')->andReturn($this->stubServices('service_2')); - $package2 = Package::new($this->stubProperties('package_2', false)) - ->addModule($module2); + $package1 = $this->stubSimplePackage('1'); + $package2 = $this->stubSimplePackage('2'); $connected = $package2->connect($package1); - $package2->boot(); + + $package2->build(); static::assertTrue($connected); - static::assertSame(['package_1' => true], $package2->connectedPackages()); + static::assertTrue($package2->isPackageConnected($package1->name())); - // we got a "not found" exception because `PackageProxyContainer::has()` return false, - // because $package1 is not booted - $this->expectExceptionMessageMatches('/not found/i'); + // We got a "not found" exception because `PackageProxyContainer::has()` return false, + // because $package1 is not built + $this->expectExceptionMessageMatches('/service_1.+not found/i'); $package2->container()->get('service_1'); } /** - * Test we can connect packages once. - * * @test */ public function testPackageCanOnlyBeConnectedOnceDebugOff(): void { - $module1 = $this->stubModule('module_1', ServiceModule::class); - $module1->expects('services')->andReturn($this->stubServices('service_1')); - $package1 = Package::new($this->stubProperties('package_1', false)) - ->addModule($module1); - - $module2 = $this->stubModule('module_2', ServiceModule::class); - $module2->expects('services')->andReturn($this->stubServices('service_2')); - $package2 = Package::new($this->stubProperties('package_2', false)) - ->addModule($module2); + $package1 = $this->stubSimplePackage('1', true); + $package2 = $this->stubSimplePackage('2', false); Monkey\Actions\expectDone($package2->hookName(Package::ACTION_PACKAGE_CONNECTED)) ->once(); Monkey\Actions\expectDone($package2->hookName(Package::ACTION_FAILED_CONNECTION)) - ->once() + ->twice() ->with($package1->name(), \Mockery::type(\WP_Error::class)); Monkey\Actions\expectDone($package2->hookName(Package::ACTION_FAILED_BUILD)) @@ -701,32 +661,23 @@ function (\Throwable $throwable): void { } ); - $connected1 = $package2->connect($package1); - + static::assertTrue($package2->connect($package1)); static::assertTrue($package2->isPackageConnected($package1->name())); - $connected2 = $package2->connect($package1); + static::assertFalse($package2->connect($package1)); + static::assertTrue($package2->isPackageConnected($package1->name())); - static::assertTrue($connected1); - static::assertFalse($connected2); + static::assertFalse($package2->connect($package1)); + static::assertTrue($package2->isPackageConnected($package1->name())); } /** - * Test we can connect packages once. - * * @test */ public function testPackageCanOnlyBeConnectedOnceDebugOn(): void { - $module1 = $this->stubModule('module_1', ServiceModule::class); - $module1->expects('services')->andReturn($this->stubServices('service_1')); - $package1 = Package::new($this->stubProperties('package_1', false)) - ->addModule($module1); - - $module2 = $this->stubModule('module_2', ServiceModule::class); - $module2->expects('services')->andReturn($this->stubServices('service_2')); - $package2 = Package::new($this->stubProperties('package_2', true)) - ->addModule($module2); + $package1 = $this->stubSimplePackage('1', false); + $package2 = $this->stubSimplePackage('2', true); Monkey\Actions\expectDone($package2->hookName(Package::ACTION_PACKAGE_CONNECTED)) ->once(); @@ -744,22 +695,18 @@ function (\Throwable $throwable): void { ); static::assertTrue($package2->connect($package1)); + static::assertTrue($package2->isPackageConnected($package1->name())); - static::expectExceptionMessageMatches('/failed connect.+?already/i'); + $this->expectExceptionMessageMatches('/already connected/i'); $package2->connect($package1); } /** - * Test we can not connect packages with themselves. - * * @test */ public function testPackageCanNotBeConnectedWithThemselves(): void { - $module1 = $this->stubModule('module_1', ServiceModule::class); - $module1->expects('services')->andReturn($this->stubServices('service_1')); - $package1 = Package::new($this->stubProperties('package_1', true)) - ->addModule($module1); + $package1 = $this->stubSimplePackage('1'); $action = $package1->hookName(Package::ACTION_FAILED_CONNECTION); Monkey\Actions\expectDone($action)->never(); @@ -768,10 +715,9 @@ public function testPackageCanNotBeConnectedWithThemselves(): void } /** - * Test getting services from a built package works, but getting services from - * a connected built package fails. + * @test */ - public function testGettingServicesFromBuiltConnectedPackageFails(): void + public function testGettingServicesFromBuiltConnectedPackage(): void { $package1 = $this->stubSimplePackage('1'); $package2 = $this->stubSimplePackage('2'); @@ -801,12 +747,9 @@ public function testGettingServicesFromBuiltConnectedPackageFails(): void static::assertSame('service_2', $container2->get('service_2')['id']); static::assertSame('service_3', $container3->get('service_3')['id']); - // And we can use Package 1 to get a service from the connected+booted Package 2 - $package1->container()->get('service_2'); - // However, we get an exception when getting a service from the connected+built Package 3 - /** TODO: Do we consider this a bug? See https://github.com/inpsyde/modularity/pull/49 */ - $this->expectExceptionMessageMatches('/service_3.+not found/i'); - $package1->container()->get('service_3'); + // And we can use Package 1 to get a service from the two connected packages + static::assertSame('service_2', $package1->container()->get('service_2')['id']); + static::assertSame('service_3', $package1->container()->get('service_3')['id']); } /**