8000 [OptionsResolver] Fix an error message to be more accurate by dimabory · Pull Request #30442 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[OptionsResolver] Fix an error message to be more accurate #30442

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 26, 2019
Merged

[OptionsResolver] Fix an error message to be more accurate #30442

merged 1 commit into from
Oct 26, 2019

Conversation

dimabory
Copy link
Contributor
@dimabory dimabory commented Mar 4, 2019
Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30432
License MIT
Doc PR

See #30432 for more details:

Symfony version(s) affected: 3.4, maybe other versions too (not tested)

Description
Error message when allowedTypes is an array contains [] but should not:
The option "testme" with value array is expected to be of type "string[]", but one of the elements is of type "integer[]".
It should be:
The option "testme" with value array is expected to be of type "string[]", but one of the elements is of type "integer".

How to reproduce

$resolver = (new OptionsResolver())
    ->setDefault('testme', [])
    ->setAllowedTypes('testme', ['string[]'])
    ->resolve(['testme' => ['test', 12]]);

In addition I changed an error message to be more
accurate if provided more than one incorrect value:

[...] is expected to be of type "integer[][]", but is of type "integer|boolean|string".

@dimabory dimabory force-pushed the options-resolver-30432 branch 2 times, most recently from 4f822e5 to 8cd409d Compare March 4, 2019 14:24
@dimabory dimabory changed the title [OptionResolver] Fix an error message to be more accurate [OptionsResolver] Fix an error message to be more accurate Mar 4, 2019
@yceruto
Copy link
Member
yceruto commented Mar 4, 2019 •
edited
Loading

There are a lot of changes unrelated to the PR' goal, which will make revision and approval difficult, I suggest you revert them and create a separate PR or issue to discuss about that? By the way, CS fixes and code refactoring should be done in highest branch (master) and lower branchs for bugfix only, that's why code version 4.2+ has improvements that 3.4 doesn't have.

@dimabory
Copy link
Contributor Author
dimabory commented Mar 4, 2019 •
edited
Loading

Why do not discuss it here?
Actually, CS fixes and refactoring are placed only in 156, 178, 573 lines. Anything else is a bugfix.

@xabbuh
Copy link
Member
xabbuh com 8000 mented Mar 4, 2019

Yes, please revert all the changes that are not necessary for the bugfix. This just makes reviewing the changes harder than needed.

@dimabory dimabory force-pushed the options-resolver-30432 branch from 8cd409d to 26fc322 Compare March 4, 2019 15:28
@dimabory
Copy link
Contributor Author
dimabory commented Mar 4, 2019

Yes, please revert all the changes that are not necessary for the bugfix. This just makes reviewing the changes harder than needed.

Done

@xabbuh xabbuh added this to the 3.4 milestone Mar 4, 2019
@dimabory dimabory force-pushed the options-resolver-30432 branch 2 times, most recently from 3b6a98e to 4b22c46 Compare March 4, 2019 16:35
yceruto
yceruto reviewed Mar 4, 2019
View reviewed changes
dimabory added a commit to dimabory/symfony that referenced this pull request Mar 6, 2019
@dimabory
symfony#30432 provide an accurate error message for multiple allowed …
ce249ba 
…types

symfony#30442 (comment)
@dimabory dimabory force-pushed the options-resolver-30432 branch from ce249ba to 16286c7 Compare March 6, 2019 15:12
dimabory added a commit to dimabory/symfony that referenced this pull request Mar 6, 2019
@dimabory
symfony#30432 provide an accurate error message for multiple allowed …
16286c7 
…types

symfony#30442 (comment)
yceruto
yceruto reviewed Mar 6, 2019
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Mar 7, 2019
View reviewed changes
@dimabory dimabory force-pushed the options-resolver-30432 branch from 16286c7 to 308ed6e Compare March 7, 2019 08:40
dimabory added a commit to dimabory/symfony that referenced this pull request Mar 7, 2019
@dimabory
symfony#30432 provide an accurate error message for multiple allowed …
308ed6e 
…types

symfony#30442 (comment)
@xabbuh
Copy link
Member
xabbuh commented Mar 7, 2019

The changes look rather complicated to me. If I am not mistaken, most of the complexity comes from composing the error message for nested arrays. I played with this a bit myself and came up with something simpler: https://github.com/symfony/symfony/compare/3.4...xabbuh:issue-30432?expand=1 Though tests don't pass with these changes yet. Would you like to give it a try?

dimabory
dimabory commented Mar 7, 2019
View reviewed changes
Copy link
Contributor Author
@dimabory dimabory left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The changes look rather complicated to me. If I am not mistaken, most of the complexity comes from composing the error message for nested arrays. I played with this a bit myself and came up with something simpler: symfony/symfony/compare/3.4...xabbuh:issue-30432?expand=1 Though tests don't pass with these changes yet. Would you like to give it a try?

Thanks! I looked at this and tried to add some test cases.

For instance [[123], ['string[]', 'string'] throws message 'The option "option" with value array is expected to be of type "string[]" or "string", but is of type "integer".'

I think we should get rid of verifyArrayType method at all (it looks weird to me and seems also too complicated).

Ping me in slack if you want to discuss it a bit more.

@dimabory dimabory force-pushed the options-resolver-30432 branch from 308ed6e to fd67b95 Compare March 11, 2019 10:50
@dimabory
Copy link
Contributor Author
dimabory commented Mar 11, 2019 •
edited
Loading

Thanks @xabbuh
symfony/symfony/compare/3.4...xabbuh:issue-30432?expand=1

Following changes in the above link, I've refactored PR's code. Seems like it contains much fewer changes and certainly easier to review now.

@dimabory dimabory force-pushed the options-resolver-30432 branch 3 times, most recently from b2721b3 to dad7b4d Compare March 11, 2019 11:12
xabbuh
xabbuh requested changes Mar 13, 2019
View reviewed changes
src/Symfony/Component/OptionsResolver/OptionsResolver.php
$fmtAllowedTypes = implode('" or "', $allowedTypes);
$fmtProvidedTypes = implode('|', array_keys($invalidTypes));

$allowedContainsArrayType = \count(array_filter(
Copy link
Member

Choose a reason for hiding this comment

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

Is this logic needed? Wouldn't something like $allowedContainsArrayType = '[]' === substr($type, -2); be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it's not enough, see test testResolveFailsIfInvalidType and data set in 575 and 576 lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xabbuh ping

@dimabory dimabory force-pushed the options-resolver-30432 branch from dad7b4d to dc4383d Compare March 13, 2019 09:54
@xabbuh xabbuh force-pushed the options-resolver-30432 branch from dc4383d to d1fb955 Compare March 27, 2019 18:51
@xabbuh
Copy link
Member
xabbuh commented Oct 15, 2019

@dimabory Sorry for the long delay. Could you please rebase here?

@dimabory dimabory force-pushed the options-resolver-30432 branch from fc4ace3 to 6addfa5 Compare October 15, 2019 12:31
@dimabory
Copy link
Contributor Author

@dimabory Sorry for the long delay. Could you please rebase here?

done...

@yceruto yceruto force-pushed the options-resolver-30432 branch from 6addfa5 to 7fa2fc2 Compare October 26, 2019 11:06
@yceruto
Copy link
Member
yceruto commented Oct 26, 2019

Thank you @dimabory.

yceruto added a commit that referenced this pull request Oct 26, 2019
@yceruto
minor #30442 [OptionsResolver] Fix an error message to be more accura…
fb70e0a 
…te (dimabory)

This PR was merged into the 3.4 branch.

Discussion
----------

[OptionsResolver] Fix an error message to be more accurate

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30432
| License       | MIT
| Doc PR        |

See #30432 for more details:
> **Symfony version(s) affected**: 3.4, maybe other versions too (not tested)
>
> **Description**
> Error message when allowedTypes is an array contains `[]` but should not:
> `The option "testme" with value array is expected to be of type "string[]", but one of the elements is of type "integer[]".`
> It should be:
> `The option "testme" with value array is expected to be of type "string[]", but one of the elements is of type "integer".`
>
> **How to reproduce**
>
> ```
> $resolver = (new OptionsResolver())
>     ->setDefault('testme', [])
>     ->setAllowedTypes('testme', ['string[]'])
>     ->resolve(['testme' => ['test', 12]]);
> ```

In addition I changed an error message to be more
accurate if provided more than one incorrect value:
> [...] is expected to be of type "integer[][]", but is of type "integer|boolean|string".

Commits
-------

7fa2fc2 #30432 fix an error message
@yceruto yceruto merged commit 7fa2fc2 into symfony:3.4 Oct 26, 2019
@dimabory dimabory deleted the options-resolver-30432 branch October 26, 2019 11:47
@nicolas-grekas
Copy link
Member

I did not manage to resolve the conflicts when merging 3.4 into 4.3 so this is not part of the branch.
PR welcome on 4.3 with the patch adapted.
Thank in advance!

@yceruto yceruto mentioned this pull request Oct 28, 2019
Merged
@yceruto
Copy link
Member
yceruto commented Oct 28, 2019

Done in #34157

nicolas-grekas added a commit that referenced this pull request Oct 28, 2019
@nicolas-grekas
minor #34157 [OptionsResolver] Fix an error message to be more accura…
7b3d5cd 
…te (yceruto)

This PR was merged into the 4.3 branch.

Discussion
----------

[OptionsResolver] Fix an error message to be more accurate

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #30432
| License       | MIT
| Doc PR        | -

Follow-up #30442 for 4.3

Commits
-------

1be68a7 Fix an error message to be more accurate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@xabbuh xabbuh xabbuh approved these changes

@yceruto yceruto yceruto approved these changes

Assignees
No one assigned
Labels
Bug OptionsResolver Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@dimabory @yceruto @xabbuh @nicolas-grekas @carsonbot
0