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.

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

DI not accepting null value of Nette config parameter if parameter is Object #283

Open
rootpd opened this issue Jun 8, 2022 · 5 comments

Comments

@rootpd
Copy link

rootpd commented Jun 8, 2022

(the original description of the issue is outdated and there's no issue with Structure, please read the discussion below)

Version: 3.0.13

Bug Description

Configuration of Nette extension can be configured by getConfigSchema() which returns Nette\Schema\Elements\Structure. The actual config is then returned as stdClass.

Consider using this config in the extension:

return Expect::structure([
    'redis_client_factory' => Expect::structure([
        'prefix' => Expect::string()->dynamic()->nullable(),
        'replication' => Expect::structure([
            'service' => Expect::string()->dynamic(),
            'sentinels' => Expect::arrayOf(Expect::string())->dynamic()
        ])
    ])
]);

If I don't provide any configuration value to the redis_client_factory.prefix, the default (null) is used. All is fine to this point.

The issue is that the DI container reports, that the redis_client_factory.prefix is missing, even if it is nullable. The reason why this happen is this snippet:

di/src/DI/Helpers.php

Lines 72 to 78 in 5f0b849

if (is_array($val) && array_key_exists($key, $val)) {
$val = $val[$key];
} elseif ($val instanceof DynamicParameter) {
$val = new DynamicParameter($val . '[' . var_export($key, true) . ']');
} else {
throw new Nette\InvalidArgumentException(sprintf("Missing parameter '%s'.", $part));
}

In the past, DI could work with the assumption that the config is always array, but it's not true anymore. Now our code throws an exception from the else branch.

  • The first condition evaluates to false, because our config ($val) is not an array, but stdClass
  • Theoretical attempt to use array_key_exists() with stdClass would work for now, but it is deprecated - you'd get "array_key_exists(): Using array_key_exists() on objects is deprecated. Use isset() or property_exists() instead".

Steps To Reproduce

The fastest would be to try to use such extension in your Nette application. Use the snippet from above in your testing extension.

FooExtension
<?php

namespace Crm\FooModule\DI;

use Nette;
use Nette\DI\CompilerExtension;
use Nette\Schema\Expect;

final class FooModuleExtension extends CompilerExtension 
{
    public function getConfigSchema(): Nette\Schema\Schema
    {
        return Expect::structure([
            'redis_client_factory' => Expect::structure([
                'prefix' => Expect::string()->dynamic()->nullable(),
                'replication' => Expect::structure([
                    'service' => Expect::string()->dynamic(),
                    'sentinels' => Expect::arrayOf(Expect::string())->dynamic()
                ])
            ])
        ]);
    }
}

Now enable the extension in your Nette application:

extensions:
	foo: Crm\FooModule\DI\FooModuleExtension

Expected Behavior

Since the property was defined as nullable(), DI should consider "no value" defaulting to null as a valid extension configuration.

Possible Solution

If I understand the code correctly, another elseif branch for stdClass using property_exists() should be sufficient, but I can't tell for sure.

Thanks for checking.

@mabar
Copy link
Contributor

mabar commented Jun 8, 2022

Your steps to reproduce show only extension registration, but not extension config, do you use any?

redis_client_factory > prefix is nullable, but redis_client_factory is not. Minimal configuration in your case should be:

foo:
  redis_client_factory:
    replication: []

By default is only structure required(), other Expect:: methods expect value only optionally.
When field is not required, it is null by default.
If null should be posible to pass explicitly (not just by default), field must be made nullable.
In compound types (arrayOf, listOf, anyOf), required() has any effect only on root element (because list/array values are validated only when exist, I am not sure about required() on an element in anyOf)

With these presumption, your minimal configuration should create following structure:

stdClass(
  redis_client_factory: stdClass(
    prefix: null
    replication: stdClass(
      service: null
      sentinels: null
    )
  )
)

Always use either nullable() or required() if you want to be sure

@rootpd
Copy link
Author

rootpd commented Jun 8, 2022

In this scenario, I didn't use any extra configuration for extension. But thanks for the clarification of the nullable and required, it was helpful.

What's more important, your comment made me realize, that this fails at the different place than I thought.

Internally, we set extra parameters to the application configuration based on the extension configuration:

public function loadConfiguration()
{
    $builder = $this->getContainerBuilder();

    // set extension parameters for use in config
    $builder->parameters['redis_client_factory'] = $this->config->redis_client_factory; // this is now stdClass

    // ...
}

Then we let other extensions to use %redis_client_factory.prefix% parameter.

So what actually happened was, that we made one of the config parameters stdClass and then tried to access one of its properties (prefix; see $var on the screenshot), which was null.

image

So the question is different.

  • Is this the expected/valid behavior? If the parameter is stdClass and the property exists, but it's null, should this be a "missing parameter" exception?
  • Or is having parameter of type stdClass completely wrong approach from our side?

@mabar
Copy link
Contributor

mabar commented Jun 8, 2022

Extension configuration can be get this way:

foreach ($this->compiler->getExtensions(FooExtension::class) as $extension) {
	$extension->getConfig();
}

Alternatively, you could get extension the same way and call some public method on it.

In an extension, parameters are already inlined where they were passed, so any changes to them will affect only extensions that access parameters directly and parameters in generated DIC. Also having an object in parameters was never expected, so that is probably why you got an exception.

Personally, I don't like extensions configuring each other, it can mess up really easy. Instead, do most of the config in neon, if it's not more complicated than in extension. Even vendor packages can have neon files, just add them via Configurator as usual.
In more complicated cases is sufficient rule to register services in loadConfiguration() in extension A and modify these services (received via $this->getContainerBuilder()->findByType()) in beforeCompile() in extension B

@mabar
Copy link
Contributor

mabar commented Jun 8, 2022

But you are right the condition is wrong. It should throw objects are not supported. Ideally I think builder parameters should be marked @readonly because only ParametersExtension should write into that property.

@rootpd
Copy link
Author

rootpd commented Jun 9, 2022

Thanks for the hints. I had a hunch that what I do is not the cleanest approach in the world, so we'll just try to amend it on our side.

The rest (@readonly for parameters, exception if not-supported stdClass is found in the parameters) sounds reasonable to me. It would make the expectations explicit and easier to understand.

@rootpd rootpd changed the title DI not accepting null value of Nette extension config if the config is Structure DI not accepting null value of Nette config parameter if parameter is Object Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants