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

Fix error on static factory with associative array #714

Merged
merged 2 commits into from
Apr 15, 2017

Conversation

ogizanagi
Copy link
Contributor

I don't know if this is something you want to officially support, but at least it used to work in previous versions ^^

User::class => [
    'user_1' => [
        '__construct' => [
            'create' => [
                'username' => 'john.doe',
                'firstname' => 'John',
                'lastname' => 'Doe',
                // ...
            ],
        ],
    ],
],

but since beta4, it'll produce the following error:

[Error]
Cannot unpack array with string keys

@theofidry
Copy link
Member

Makes sense, I simply overlooked it when I did #698. Do you mind adding some tests in LoaderIntegrationTest as well? You should have some examples in the linked PR.

@theofidry theofidry added this to the 3.0.0 milestone Apr 14, 2017
@ogizanagi
Copy link
Contributor Author

I tried adding one, but it fails with a Nelmio\Alice\Throwable\Exception\FixtureBuilder\Denormalizer\UnexpectedValueException: Could not denormalize the given constructor exception :/

I don't really get what's happening here, also considering the fix is working properly on my own project and beta5. 😕

@ogizanagi
Copy link
Contributor Author

ogizanagi commented Apr 14, 2017

I managed in ba986d0 to fix the tests by injecting & using the arguments denormalizer in ConstructorWithCallerDenormalizer, but I don't know if these changes are legit.

BTW, ideally, shouldn't ConstructorWithCallerDenormalizer be able to decorate any ConstructorDenormalizerInterface, and not just a SimpleConstructorDenormalizer ?

@theofidry
Copy link
Member

theofidry commented Apr 14, 2017

Hm no that doesn't seem right: alice is broken down in 2 steps: creating the fixture objects and generating the actual objects. The first part is supposed to be idempotent and immutable. They shouldn't be mixed. More details there.

I cannot really debug it this we, but I would rather say ArgumentsDenormalizer or one of it's caller is likely to be the problem

@ogizanagi
Copy link
Contributor Author

Are you sure those changes aren't right? It only updates the first part about generating the fixture objects and to me does not violates the idempotent and immutable rules.
It looked actually weird to try back using the SimpleConstructorDenormalizer in the case of a factory method, just to get the arguments fixture objects and build a MethodCallWithReference from it.

To me the ConstructorWithCallerDenormalizer should:

  1. Try using the decorated implementation (here SimpleConstructorDenormalizer). If it succeed, just return, if it fails, continue.
  2. Resolve the caller reference (here simply the current classname in the scope) and method.
  3. Denormalize the arguments. Previous code was using the simpleConstructorDenormalizer for this purpose. But why? We just need to denormalize arguments, we know how to build a MethodCallInterface instance from previously collected informations.
  4. Build and return the MethodCallWithReference.

Maybe I'm missing something 😕

@theofidry
Copy link
Member

Ah, I actually completely misread your change 😅

@theofidry theofidry merged commit 8f9f1bf into nelmio:master Apr 15, 2017
@theofidry
Copy link
Member

Thanks @ogizanagi :)

@ogizanagi ogizanagi deleted the fix/static_factory_arg_keys branch April 15, 2017 10:37
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.

None yet

2 participants