Description of the bug

When having a child theme, ThemeProperties::basePath() returns the parent theme's path instead of the child theme's.

Reproduction instructions

1. Create a child theme
2. Initialize the package properties with

        $properties = ThemeProperties::new('child-theme-name');
        $package = Package::new($properties);

3. Boot the package
4. Use ThemeProperties::basePath()

Expected behavior

If parent theme (let's call it parent-theme) has a child theme (let's call it child-theme), then when calling ThemeProperties::basePath() from within child-theme I expect to see a path of .../wp-content/themes/child-theme.
Instead I get .../wp-content/themes/parent-theme.

Environment info

Package version: 1.6.0
Browser: N/A
System: Windows 10

Relevant log output

No response

Additional context

This is caused because properties are initialized using get_template_directory() instead of get_stylesheet_directory().

Code of Conduct

• I agree to follow this project's Code of Conduct

This line broke my plugin which uses a newer version of Psr\Container\ContainerInterface because it doesn't have a return type of bool. An update would be nice.

Describe the bug
At least in the documentation (https://github.com/inpsyde/modularity/blob/master/docs/Properties.md) and the release notes for v1.3.0 (https://github.com/inpsyde/modularity/releases/tag/1.3.0) AuthorURI is mentioned as one of the plugin headers. However, similar to the theme headers, it is Author URI (with whitespace), see https://developer.wordpress.org/plugins/plugin-basics/header-requirements/#header-fields.

Expected behavior
All examples should use the official plugin header Author URI instead of AuthorURI.

Additional context
I haven't checked the actual code, so I don't know if this is just a typo in the documentation or if the code actually doesn't support Author URI.

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.

Description of the bug

Running tests with PHP 8.2 I'm getting an error:
Error: This version of PHPUnit does not support code coverage on PHP 8

Also some tests fail out of the box:

1. Inpsyde\Modularity\Tests\Unit\Properties\PluginPropertiesTest::testIsMuPlugin with data set "is not mu-plugin" ('/wp-content/plugins/the-plugin/', '/wp-content/mu-plugins/', false)

2. Inpsyde\Modularity\Tests\Unit\Properties\PluginPropertiesTest::testIsMuPlugin with data set "is mu-plugin" ('/wp-content/mu-plugins/the-plugin/', '/wp-content/mu-plugins/', true)

Reproduction instructions

1. git clone https://github.com/inpsyde/modularity.git
2. cd modularity
3. composer install
4. vendor/phpunit/phpunit/phpunit

Expected behavior

All unit tests pass

Environment info

• dev-master
• php 8.2.5

Relevant log output

No response

Additional context

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

This issue describes an additional funcitonality that I think would be nice to have, based on the experience of using WP App Container for plugins and from discussion with colleagues trying to implement modularity in their plugins.

The scenario

Let's imagine we have a simple plugin that needs to perform a couple of operations.

The first operation must be done pretty early, let's say on "init".

The second operation can't happen that early, because it depends on a condition that is only possible to check at a later hook.

The plugin could use two modules, one per operation.

Let's ignore for now the first module, and let's imagine how the second module could look like.

class MyTemplateRedirectModule implements ServiceModule, ExecutableModule
{
    public function services(): array
    {
       // register here a few services
    }

    public function run(ContainerInterface $c): bool
    {
       return add_action(
           'template_redirect',
           function () use ($c) {
               if ( ! is_post_type_archive(MyCpt::NAME) ) {
                   return;
               }

               // do things here using services from container
           }
       );
    }
}

This a quite common situation. We only need to do "things" on specific type of frontoffice request, but the first hook to safely check for conditional tags is template_redirect.

The issue

Now, if in our website we have a request that satisfies our condition once every 1000 requests, it means that 99.9% of the times the module registers services in the container for no reason.

When the number of services registered is not trivial, register services might be resource consuming, because pollutes memory and trigger autoload (which might be slow file-based operation).

When template_redirect runs, it is too late to add things in the container, because the plugin boot is executed earlier (because there's a plugin functionality that needs to run earlier).

That's why we check the condition inside run(), but the container instance passed there is read-only, so we need to add services earlier.

In short, the problem is that when we have a module that should register things conditionally, and the condition is not available very soon (quite common in WordPress), we are forced to register things unconditionally and then check the condition later.

The possible solution

I think that a possible solution could be to introduce a DeferredModule interface, similar to this:

interface DeferredModule extends Module
{
    public function deferredTo(): string;

    public function factoryModule(Package $package): ?Module;
}

and a Package::addDeferredModule() method, with a signature like this:

public function addDeferredModule(DeferredModule $module): Package;

Internally, the method would add an action to the hook returned by DeferredModule::deferredTo() and inside that hook, it would call DeferredModule::factoryModule() and the returned module, if any, would be added in the same way it is hadded when calling Package::addModule().

The code could look like this (it's an idea, completely untested):

public function addDeferredModule(DeferredModule $module): Package
{
        add_action(
            $module->deferredTo(),
            function () use ($moduleFactory) {
                $deferredModule = $module->factoryModule($this);
                if (!$deferredModule || $this->statusIs(self::STATUS_FAILED)) {
                    return;
                }

                // services/factories/extensions are added here...

                if (($deferredModule instanceof ExecutableModule) && $this->statusIs(self::STATUS_BOOTED)) {
                    // defered + executable module is executed right-away, when package was already booted
                    $module->run($this->container());
                }
            }
        );
    }

With the above code in place, in the scenario I described a the beginning, the MyTemplateRedirectModule could be rewritten like this:

class MyTemplateRedirectModule implements ServiceModule, ExecutableModule, DeferredModule
{
    public function deferredTo(): string
    {
        return  'template_redirect';
    }

    public function factoryModule(Package $package): ?Module
    {
       return is_post_type_archive(MyCpt::NAME) ? $this : null;
    }

    public function services(): array
    {
       // register here a few services
    }

    public function run(ContainerInterface $c): bool
    {
        // do things here using services from container
    }
}

In this case the deferred module return itself from factoryModule(), but it could return another module.

Anyway, having the proposed code in place, by calling the following line the issue is solved in a pretty simple way.

$package->addDeferredModule(new MyTemplateRedirectModule());

For requests that are not archive of a specific post type, not only the module does not execute anything, it also does not register anything.

Additional notes

Considerations on addDeferredModule()

The code proposed above for addDeferredModule() is simplified. There are other things to consider, e.g.:

• which priority to use when adding the hook for deferred module? We could accept an additional $priority parameter in addDeferredModule, or maybe have another PrioritizedDeferredModule interface which would extend DeferredModule with an additional priority() method.
• In some situations there's not a viable action hook to use, but only a filter. So it might be required to use add_filter instead of add_action. We could accept an additional $isFilter parameter in addDeferredModule, or maybe have another FilterHookDeferredModule interface which would extend DeferredModule to distinguish actions from filters.
• If it is decided to don't use additional parameters for addDeferredModule(), but use additioanl interfaces for the two reasosn exposed in the previous two points, the addDeferredModule() would have the same signature of addModule() so they can be merged in a single method, and based on the interface apply the deferred workflow.

Deferred modules for inter-module dependency

Deferred modules could be used for another use case, thanks to the fact that the DeferredModule::factoryModule() receives an instance of the package.

Imagine a module that should be added only if another module is present.

Package class has the moduleIs() method, but that can't be used on Package::ACTION_INIT to determine if a given package was added or not, because maybe the target package will be added later. And in any case, can't be used to check if a module was executed or not, because the execution happens later.

With the addition of the proposed functionalities, it would be possible to do things like:

class SomeModuleDependantModule implements ServiceModule, ExecutableModule, DeferredModule
{
    public function __construct(string $hookName)
    {
        $this->hookName = $hookName;
    }

    public function deferredTo(): string
    {
        return  $this->hookName;
    }

    public function factoryModule(Package $package): ?Module
    {
        return $package->moduleIs(SomeModule::class, Package::MODULE_EXECUTED)
            ? $this
            : null;
    }

    public function services(): array
    {
       // register here a few services
    }

    public function run(ContainerInterface $c): bool
    {
        // do things here using services from container
    }
}

$plugin = plugin();
$module = new SomeModuleDependantModule($plugin->hookName(Package::ACTION_READY));
$plugin->addDeferredModule($module);

In the snippet above, I added a deferred module that when the package is ready (ACTION_READY action is fired) checks if the SomeModule module was executed successfully, and only if so will add itself as an additional module.

A similar workflow can be currently implemented. In fact, currently it is possible to do:

add_action(
    $package->hookName(Package::ACTION_READY)
    function (Package $package) {
        if ($package->moduleIs(SomeModule::class, Package::MODULE_EXECUTED)) {
            (new SomeModuleDependantModule())->run()
        }
    }
);

So we can currently execute SomeModuleDependantModule only if SomeModule was executed successfully, but we can't currently add services at that point, because when ACTION_READY hook is fired, the container is read-only, so any service required by SomeModuleDependantModule would need to be registered unconditionally.

Is your feature request related to a problem? Please describe.
I want to get the WC requires at least field value, that is not provided by properties. I tried to extend PluginProperties and overwrite HEADERS constant, but it's not worked because it's called in the constructor as self::HEADERS , so the constant is used from the PluginProperties class, not from the extending one.

Describe the solution you'd like
It would be great to have an option to get value of any of WP plugin header fields with Properties::get(). Probably, just merge everything returned by the get_plugin_data() function, as discussed in Slack.

Describe alternatives you've considered
Another option is to allow classes extending the PluginProperties to overwrite the parsed headers list. For example, use the protected method headers() instead of a class constant. Or, it should be possible to override that constant if it's used as static::HEADERS instead of self::HEADERS.

Additional context
This is how it's expected to work. Plugin header:

/**

• Plugin Name: My plugin
• Description: This plugin does something
• Version: 1.0.0
  ...
• WC requires at least: 5.0
  */

Usage of properties:

    $pluginProperties->get('WC requires at least'); //5.0

or if this property was configured in custom HEADERS constant:

Usage of properties:

    $pluginProperties->get('wcRequiresAtLeast'); //5.0

Is your feature request related to a problem? Please describe.
Currently our code restricts Services to be object when get and resolving internally the extensions via ReadOnlyContainer::resolveExtensions(string $id, object $service): object. This happens for Services comming from our own Modules but also for Services comming from external Containers

This currently does not allow full support for external PSR Containers, which do support mixed return values (eventho its not recommanded or offically documented).

Examples:

• PHP DI: https://php-di.org/doc/php-definitions.html#values
• thephpleague/container - https://github.com/thephpleague/container/blob/3.x/src/Definition/Definition.php#L223

Describe the solution you'd like
We should remove the object typehint on $service and also on the return type for this method. Additionally we should update the documentation to mention support of non-object values for Services with clarification that we do not recommand this.