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(): Fix the missing resolving or references in simple string arrays #984

Merged

Conversation

TiMESPLiNTER
Copy link
Contributor

I had the problem that references in an array:

['@user1', '@user2']

were correctly recognized but never resolved to a User object but stayed as a reference object in the data structure. This PR fixes this.

@theofidry
Copy link
Member

Thanks very much for the fix! Could you add a test as well to LoaderIntegrationTest? Your patch should be enough strictly speaking but I prefer to add an extra safety with an integration test

@TiMESPLiNTER
Copy link
Contributor Author

TiMESPLiNTER commented Apr 2, 2019

@theofidry isn't this the test already? https://github.com/nelmio/alice/blob/master/tests/Loader/LoaderIntegrationTest.php#L2546

It looks like exactly what I need. Just asking myself why this test passes in the current master. 🤔

@theofidry
Copy link
Member

Ah indeed, then could it be that the reproducer is not just having a plain array? Do you have a unique property or something with it?

@TiMESPLiNTER
Copy link
Contributor Author

 'another_dummy' => [
    'dummies' => ['@dummy', '@dummy', '@dummy'],
],

this is a plain array isn't it? Or maybe I don't get exactly what you mean by "plain array"...

@theofidry
Copy link
Member

theofidry commented Apr 2, 2019

this is a plain array isn't it? Or maybe I don't get exactly what you mean by "plain array"...

Yes it is.

There is several reports (like #973) which hints there is an issue with some values which are not properly resolved, so I don't doubt you encountered that situation. I just think the failing scenario is not the one you original described. But maybe the other linked issue can give an idea of what can cause this failure, and once found, we can add a test to LoaderIntegrationTest to make sure this scenario is properly fixed

@TiMESPLiNTER
Copy link
Contributor Author

@theofidry yes, issue #973 was exactly the problem I solved with this PR now. So #973 will also be fixed once this gets merged. It must be something different as the test I mentioned passes in master without problems. Although I can't come up with another idea which constellation reproduces the issues mentioned here and in #973. So I know what causes the failure described there and this PR fixes it.

@theofidry
Copy link
Member

theofidry commented Apr 2, 2019

Could you try:

yield 'array value' => [
            [
                stdClass::class => [
                    'dummy' => [
                        'foo' => 'bar',
                    ],
                    'another_dummy' => [
                        'dummies' => '[@dummy, @dummy, @dummy]',
                    ],
                ],
            ],
            [
                'parameters' => [],
                'objects' => [
                    'dummy' => $dummy = StdClassFactory::create([
                        'foo' => 'bar',
                    ]),
                    'another_dummy' => StdClassFactory::create([
                        'dummies' => [$dummy, $dummy, $dummy]
                    ]),
                ],
            ],
        ];

or:

yield 'array value' => [
            [
                stdClass::class => [
                    'dummy' => [
                        'foo' => 'bar',
                    ],
                    'another_dummy' => [
                        'dummy' => '<randomElement([@dummy, @dummy, @dummy])>',
                    ],
                ],
            ],
            [
                'parameters' => [],
                'objects' => [
                    'dummy' => $dummy = StdClassFactory::create([
                        'foo' => 'bar',
                    ]),
                    'another_dummy' => StdClassFactory::create([
                        'dummy' => $dummy,
                    ]),
                ],
            ],
        ];

without your patch first to see it if makes it fail and then with your patch to see if it solves the issue?

@TiMESPLiNTER
Copy link
Contributor Author

Nice the first one works. Fails without my change and passes with my change. 👍 I just pushed it into this branch.

@theofidry theofidry merged commit 8678b22 into nelmio:master Apr 2, 2019
@theofidry
Copy link
Member

Thanks!

@TiMESPLiNTER TiMESPLiNTER deleted the fix/non-resolved-ref-in-arrays branch April 2, 2019 15:07
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.

2 participants