Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aligned behavior using build #45

Closed
wants to merge 2 commits into from
Closed

Aligned behavior using build #45

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99bc6c0
feat: aligned behavior using build
luislard Apr 28, 2024
4f1825c
Update tests/unit/PackageTest.php
Chrico May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Container/PackageProxyContainer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to differentiate between STATUS_INITIALIZED and an eventual new STATUS_BUILT or we can say that STATUS_INITIALIZED previsely means that package is built?

In that second case, should we as an STATUS_BUILT as an alias for STATUS_INITIALIZED? And maybe even deprecate the latter?

Alos, I see you have introduced the flag built in this commit, and you use it in the build() method to check the status. If there's a good reason for doing that instead of checking the status, why don't do it as well here?

|| $this->package->statusIs(Package::STATUS_READY)
|| $this->package->statusIs(Package::STATUS_BOOTED)
) {
$this->container = $this->package->container();
Expand All @@ -88,7 +89,6 @@ private function assertPackageBooted(string $id): void
if ($this->tryContainer()) {
return;
}

$name = $this->package->name();
$status = $this->package->statusIs(Package::STATUS_FAILED)
? 'is errored'
Expand Down
18 changes: 13 additions & 5 deletions src/Package.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,10 +305,14 @@ public function connect(Package $package): bool
throw new \Exception($error, 0, $this->lastError);
}

// Don't connect, if already booted or boot failed
// Don't connect, if status is failed
$failed = $this->statusIs(self::STATUS_FAILED);
if ($failed || $this->statusIs(self::STATUS_BOOTED)) {
$status = $failed ? 'errored' : 'booted';
// Don't allow connect if current package is already built
if ($failed
|| $this->statusIs(self::STATUS_INITIALIZED) // built
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above: in the build() method you introduced check for "built" status checking for $this->built. Would it make sense to do this here as well?

|| $this->statusIs(self::STATUS_BOOTED) // booted
) {
$status = $failed ? 'errored' : 'built_or_booted';
$error = "{$errorMessage} to a {$status} package.";
do_action(
$this->hookName(self::ACTION_FAILED_CONNECTION),
Expand All @@ -330,14 +334,15 @@ static function () use ($package): Properties {
}
);

// If the other package is booted, we can obtain a container, otherwise
// If the other package is built or booted, we can obtain a container, otherwise
// we build a proxy container
$container = $package->statusIs(self::STATUS_BOOTED)
|| $package->statusIs(self::STATUS_INITIALIZED)
Copy link
Contributor

Choose a reason for hiding this comment

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

And again ssame question asked twice above.

// || $package->statusIs(self::STATUS_READY)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this line commented?

I think it makse sense to check for this status, and if we're sure it doesn't, the we should remove the line not comment it.

? $package->container()
: new PackageProxyContainer($package);

$this->containerConfigurator->addContainer($container);

do_action(
$this->hookName(self::ACTION_PACKAGE_CONNECTED),
$packageName,
Expand All @@ -361,6 +366,9 @@ static function () use ($package): Properties {
*/
public function build(): Package
{
if ($this->built) {
return $this;
}
Comment on lines +369 to +371
Copy link
Contributor

Choose a reason for hiding this comment

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

First of all, I want to ask what is the reason for this lines.

This allows for calling build() multiple times idempotently. I'm not necessarily against that, but would like to understand the reasoning.

None of the other public API of this class (boot(), connect()) is idempotent, expecting specific methods to be called at specific steps in the object life cycle.

So my questions are:

  • Why this method is different?
  • If there're good reasons for idempotence, do those reasons apply to other methods as well?

When we started this class, we thought about having specifc flags for different statuses (idle, ready, failed) but then we opted to have a single status property. Why is "built" a status special in this regard? Why can't we add a built status instead fo an explicit flag?

I already asked above: do we need to differentiate between STATUS_INITIALIZED and an eventual new STATUS_BUILT or we can say that STATUS_INITIALIZED previsely means that package is built?

  • If we don't need to differentiate because STATUS_INITIALIZED means built, can't we check $this->statusIs(self::STATUS_INITIALIZED) instead of using this flag?
  • If we need to differentiate, the question is the same as above, why don't we introduce that new status and check it via statusIs()?
  • If there's a good reason to have the flag instead of checking the status, why in other places are we checking the status instead of the flag for the same purpose?

try {
// Don't allow building the application multiple times.
$this->assertStatus(self::STATUS_IDLE, 'build package');
Expand Down
205 changes: 205 additions & 0 deletions tests/unit/PackageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,25 @@ public function testBasic(): void
static::assertEmpty($package->modulesStatus()[Package::MODULES_ALL]);
}

/**
* @test
*/
public function testBasicUsingBuild(): void
{
$expectedName = 'foo';
$propertiesStub = $this->mockProperties($expectedName);

$package = Package::new($propertiesStub);

static::assertTrue($package->statusIs(Package::STATUS_IDLE));
static::assertInstanceOf(Package::class, $package->build());
static::assertTrue($package->statusIs(Package::STATUS_INITIALIZED));
static::assertSame($expectedName, $package->name());
static::assertInstanceOf(Properties::class, $package->properties());
static::assertInstanceOf(ContainerInterface::class, $package->container());
static::assertEmpty($package->modulesStatus()[Package::MODULES_ALL]);
}

/**
* @param string $suffix
* @param string $baseName
Expand Down Expand Up @@ -105,6 +124,32 @@ public function testBootWithEmptyModule(): void
static::assertFalse($package->boot());
}

/**
* @test
*/
public function testBuildWithEmptyModule(): void
{
$expectedId = 'my-module';

$moduleStub = $this->mockModule($expectedId);
$propertiesStub = $this->mockProperties('name', false);

$package = Package::new($propertiesStub)->addModule($moduleStub);

static::assertInstanceOf(Package::class, $package->build());
static::assertTrue($package->moduleIs($expectedId, Package::MODULE_NOT_ADDED));
static::assertFalse($package->moduleIs($expectedId, Package::MODULE_REGISTERED));
static::assertFalse($package->moduleIs($expectedId, Package::MODULE_REGISTERED_FACTORIES));
static::assertFalse($package->moduleIs($expectedId, Package::MODULE_EXTENDED));
static::assertFalse($package->moduleIs($expectedId, Package::MODULE_ADDED));

// build again should keep the status of the package.
Copy link
Contributor

Choose a reason for hiding this comment

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

I've asked in another place, but here I also don't see a reasoning.

Why it is important that we can call $package->build() multiple tiems idempotently, while for example, we can't do that for boot() or connect().

And again, I'm not necessarily againts this, I just want to understand what makes build() special in this regard.

static::assertTrue($package->statusIs( Package::STATUS_INITIALIZED));
$package->build();
static::assertTrue($package->statusIs( Package::STATUS_INITIALIZED));

}

/**
* @test
*/
Expand All @@ -127,6 +172,28 @@ public function testBootWithServiceModule(): void
static::assertTrue($package->container()->has($serviceId));
}

/**
* @test
*/
public function testBootWithServiceModuleUsingBuild(): void
{
$moduleId = 'my-service-module';
$serviceId = 'service-id';

$module = $this->mockModule($moduleId, ServiceModule::class);
$module->expects('services')->andReturn($this->stubServices($serviceId));

$package = Package::new($this->mockProperties())->addModule($module);

static::assertInstanceOf(Package::class, $package->build());
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_NOT_ADDED));
static::assertTrue($package->moduleIs($moduleId, Package::MODULE_REGISTERED));
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_REGISTERED_FACTORIES));
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_EXTENDED));
static::assertTrue($package->moduleIs($moduleId, Package::MODULE_ADDED));
static::assertTrue($package->container()->has($serviceId));
}

/**
* @test
*/
Expand All @@ -149,6 +216,28 @@ public function testBootWithFactoryModule(): void
static::assertTrue($package->container()->has($factoryId));
}

/**
* @test
*/
public function testBootWithFactoryModuleUsingBuild(): void
{
$moduleId = 'my-factory-module';
$factoryId = 'factory-id';

$module = $this->mockModule($moduleId, FactoryModule::class);
$module->expects('factories')->andReturn($this->stubServices($factoryId));

$package = Package::new($this->mockProperties())->addModule($module);

static::assertInstanceOf(Package::class, $package->build());
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_NOT_ADDED));
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_REGISTERED));
static::assertTrue($package->moduleIs($moduleId, Package::MODULE_REGISTERED_FACTORIES));
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_EXTENDED));
static::assertTrue($package->moduleIs($moduleId, Package::MODULE_ADDED));
static::assertTrue($package->container()->has($factoryId));
}

/**
* @test
*/
Expand All @@ -172,6 +261,29 @@ public function testBootWithExtendingModuleWithNonExistingService(): void
static::assertFalse($package->container()->has($extensionId));
}

/**
* @test
*/
public function testBuildWithExtendingModuleWithNonExistingService(): void
{
$moduleId = 'my-extension-module';
$extensionId = 'extension-id';

$module = $this->mockModule($moduleId, ExtendingModule::class);
$module->expects('extensions')->andReturn($this->stubServices($extensionId));

$package = Package::new($this->mockProperties())->addModule($module);

static::assertInstanceOf(Package::class, $package->build());
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_NOT_ADDED));
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_REGISTERED));
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_REGISTERED_FACTORIES));
static::assertTrue($package->moduleIs($moduleId, Package::MODULE_EXTENDED));
static::assertTrue($package->moduleIs($moduleId, Package::MODULE_ADDED));
// false because extending a service not in container
static::assertFalse($package->container()->has($extensionId));
}

/**
* @test
*/
Expand All @@ -195,6 +307,29 @@ public function testBootWithExtendingModuleWithExistingService(): void
static::assertTrue($package->container()->has($serviceId));
}

/**
* @test
*/
public function testBuildWithExtendingModuleWithExistingService(): void
{
$moduleId = 'my-extension-module';
$serviceId = 'service-id';

$module = $this->mockModule($moduleId, ServiceModule::class, ExtendingModule::class);
$module->expects('services')->andReturn($this->stubServices($serviceId));
$module->expects('extensions')->andReturn($this->stubServices($serviceId));

$package = Package::new($this->mockProperties())->addModule($module);

static::assertInstanceOf(Package::class, $package->build());
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_NOT_ADDED));
static::assertTrue($package->moduleIs($moduleId, Package::MODULE_REGISTERED));
static::assertFalse($package->moduleIs($moduleId, Package::MODULE_REGISTERED_FACTORIES));
static::assertTrue($package->moduleIs($moduleId, Package::MODULE_EXTENDED));
static::assertTrue($package->moduleIs($moduleId, Package::MODULE_ADDED));
static::assertTrue($package->container()->has($serviceId));
}

/**
* @test
*/
Expand Down Expand Up @@ -497,6 +632,38 @@ public function testPackageConnection(): void
static::assertInstanceOf(\ArrayObject::class, $package2->container()->get('service_1'));
}

/**
* Test we can connect services across packages.
*
* @test
*/
public function testPackageConnectionWhenOnePackageIsBuiltAndTheOtherDont(): 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, false);

$package1->build();

$connected = $package2->connect($package1);
$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.
*
Expand Down Expand Up @@ -535,6 +702,44 @@ function (\Throwable $throwable): void {
static::assertSame(['package_1' => false], $package2->connectedPackages());
}

/**
* Test we can not connect services when the package how call connect is built.
Chrico marked this conversation as resolved.
Show resolved Hide resolved
*
* @test
*/
public function testPackageConnectionFailsIfBuiltWithDebugOff(): 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->build();
$package2->build();

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.+?built');
}
);

$connected = $package2->connect($package1);

static::assertFalse($connected);
static::assertSame(['package_1' => false], $package2->connectedPackages());
}

/**
* Test we can not connect services when the package how call connect is booted.
*
Expand Down
Loading