Inconsistent Package status

[gmazzap]

Describe the bug

Assume this module class:

use Inpsyde\Modularity\Module\{
    Module,
    ServiceModule,
    ExtendingModule,
    FactoryModule
};

class MyModule implements Module, ServiceModule, ExtendingModule, FactoryModule
{
    public function id(): string { return 'test' }

    public function services(): array { return [] }

    public function extensions(): array { return [] }

    public function factories(): array { return [] }
}

and assume the following code:

use Inpsyde\Modularity as _;

$prop = _\Properties\LibraryProperties::new('/path/to/composer.json');
$package = _\Package::new($prop);

$package->addModule(new MyModule); // see previous snippet

print_r($package->modulesStatus());

What do you expect the print_r outputs?

I would have expected nothing, because the module does literally nothing.

But its actual output is:

Array (
    [*] => Array (
        [0] => test registered.
        [1] => test registered.
        [2] => test extended.
        [3] => test added.
    )
    [added] => Array (
        [0] => test
    )
    [registered] => Array (
        [0] => test
        [1] => test
    )
    [extended] => Array (
        [0] => test
    )
    [executed] => Array ()
    [executed-failed] => Array ()
)

The two issues

This to me means there are two issues:

1. a module is which is both ServiceModule and FactoryModule will be added as "registered" twice. And that both in the registered key and in the * key.
2. a module is considered as "registered" / "added" / "extended" as soon as it implements the interface, not if it actually registers / add / extends things in the container.

First issue

the reason is that Package class calls moduleProgress() two times with the same parameters (see here and here).

This could be solved in two ways:

• add another MODULE_* constant, e.g. MODULE_REGISTERED_FACTORIES
• alternatively, don't call moduleProgress() when registering factories if it was already called when registering regular services.

Which do you prefer?

Second issue

this might be misleading for debug.

Imagine a method like:

    public function services(): array
    {
        if (some_condition()) {
            return [];
        }

        return all_the_services();
    }

No matter if some_condition() will be true or false, $package->modulesStatus() will show the module as "registered" so one might think that services where registered, but maybe they were not.

To solve this, moduleProgress() should be called only if some services / factories / extensions are actually returned.

E.g. the current code:

if ($module instanceof ServiceModule) {
    foreach ($module->services() as $serviceName => $callable) {
        $this->containerConfigurator->addService($serviceName, $callable);
    }
    $added = true;
    $this->moduleProgress($module->id(), self::MODULE_REGISTERED);
}

could become:

if ($module instanceof ServiceModule) {
    $services = $module->services();
    if ($services) {
        foreach ($services as $serviceName => $callable) {
            $this->containerConfigurator->addService($serviceName, $callable);
        }
        $added = true;
        $this->moduleProgress($module->id(), self::MODULE_REGISTERED);
    }
}

and the same for FactoryModule and ExecutableModule.

What's next

If you agree, I'll be happy to send a PR, but I would need to know what to do about the duplicated "registered" entry (de-duplicate or introduce MODULE_REGISTERED_FACTORIES)

To Reproduce

Execute the code I posted above :)

System

Unrelated.

Additional context

None.

[Chrico]

Thanks for your feedback 👍 The raised issues and solutions are sounding reasonable for us. If you want you can go ahead and..

a) Update the Package::addModule() to check if actually services/extension/factories are returned. b) Introduce a new class constant for registered factories to be more clear.

Maybe we could even extend Package::moduleProgress() by a thrid argument to contain the id's of the services registered for the current Module. What do you think?

[gmazzap]

Maybe we could even extend Package::moduleProgress() by a thrid argument to contain the id's of the services registered for the current Module.

It makes sense, but my fear is that when we'll have many packages and a total of hundreds of services the memory footprint will raise too much. Even because each service ID will be a pretty long string.

Maybe we could store services IDs only if debug is true?

[Chrico]

Maybe we could store services IDs only if debug is true?

Would make sense, we have access to the Properties and therfor access to Properties::isDebug(). 👍

[gmazzap]

Fixed via #11