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

Bugfix: anonymous commands can't have same argument sequence #45

Merged
merged 1 commit into from
Jan 20, 2016
Merged

Bugfix: anonymous commands can't have same argument sequence #45

merged 1 commit into from
Jan 20, 2016

Conversation

foglcz
Copy link
Contributor

@foglcz foglcz commented Jan 19, 2016

Type Bugfix
Fixes issues n/a - not reported
Documentation not needed
BC Break no
Tests updated yes

When we register commands as anonymous services with parameters, they get registered in container with name "command(md5hash)". The MD5 is constructed from json_encode, which encodes instance of Nette\DI\Statement.

If we have following .neon file configuration (real world example):

console:
    commands:
        - App\Console\SynchronizeToOI(@shopDibi, @oiDibi)
        - App\Console\SynchronizeToShops(@shopDibi, @oiDibi)

The second command fails to register with message Service 'console.command.b84a00605660d460bba12d52a02bf1d8' has already been added.

This is because the md5 hash actually hashes only public properties of the Statement - which does not contain the class name.

Test code in ConsoleExtension.php:78

        foreach ($config['commands'] as $command) {
            echo Nette\Utils\Json::encode($command) . " ---> ";
            echo md5(Nette\Utils\Json::encode($command)) . "\n";
        }

Test output

C:\Users\pptacek\www\projectname>php www/index.php
{"arguments":["@shopDibi","@oiDibi"]} ---> b84a00605660d460bba12d52a02bf1d8
{"arguments":["@shopDibi","@oiDibi"]} ---> b84a00605660d460bba12d52a02bf1d8

Effectively, all anonymous commands with constructor-inject, were hashed solely based on their parameter sequence.

Fixed debug code:

        foreach ($config['commands'] as $command) {
            $entityName = ($command instanceof Statement) ? $command->getEntity() : '';
            echo $entityName.Nette\Utils\Json::encode($command) . " ---> ";
            echo md5($entityName.Nette\Utils\Json::encode($command)) . "\n";
        }
        exit;

Fixed debug output:

C:\Users\pptacek\www\projectname>php www/index.php
App\Console\SynchronizeToOI{"arguments":["@shopDibi","@oiDibi"]} ---> c2af7ff37cb79a56f83aaf2a40874694
App\Console\SynchronizeToShops{"arguments":["@shopDibi","@oiDibi"]} ---> 2636b2fdc12987674438dd205cf53208

Implementation note the md5 hash can be fixed by using var_export or print_r($entity, true) instead of JSON. Since JSON has been selected originally, I tried to stick with something compact & human-readable (thus prepending the name if working with Statement).

@@ -81,7 +81,8 @@ public function loadConfiguration()

Nette\Utils\Validators::assert($config, 'array');
foreach ($config['commands'] as $command) {
$def = $builder->addDefinition($this->prefix('command.' . md5(Nette\Utils\Json::encode($command))));
$entityName = ($command instanceof Statement) ? $command->getEntity() : '';
$def = $builder->addDefinition($this->prefix('command.' . md5($entityName.Nette\Utils\Json::encode($command))));
Copy link
Member

Choose a reason for hiding this comment

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

I don't even remember why I did it in this way. Maybe this would be enough?

foreach ($config['commands'] as $i => $command) {
    $def = $builder->addDefinition($this->prefix('command.' . $i));

Copy link
Contributor

Choose a reason for hiding this comment

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

@fprochazka 👍 should be enough and is much simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! I think it might have been premature prevention of possible conflicts with master service container (which was my first thought and worry.) After testing though, all seems ok and working no matter what I throw at it (combined services alongside with console section, and DI seems happy.)

Updated, squashed, push --forced the commit and now we should be good to go :)

fprochazka added a commit that referenced this pull request Jan 20, 2016
Bugfix: anonymous commands can't have same argument sequence
@fprochazka fprochazka merged commit c1296c4 into Kdyby:master Jan 20, 2016
@fprochazka
Copy link
Member

@foglcz thank you very much! And amazing report by the way :)

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

Successfully merging this pull request may close these issues.

4 participants