Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[OptionsResolver] support array of instance validation#17032

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

@EVODelavega
Copy link
Contributor

QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#15524
LicenseMIT
Doc PRsymfony/symfony-docs#6048

A suggested approach to add support for typed-arrays as allowed types, using a simple recursive method.

ClosingPR 15671.

sstok reacted with heart emoji
@EVODelavega
Copy link
ContributorAuthor

PR was reviewed in previous PR, so setting status to reviewed now
Status: Reviewed

Copy link
Member

Choose a reason for hiding this comment

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

is

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say "Whether the $value is of the allowed type".

@stof
Copy link
Member

there is a bunch of unrelated changes in this PR, making me think you messed the history a bit.

@nicolas-grekas
Copy link
Member

Status: needs work (git history rebasing+squashing)

@soullivaneuh
Copy link
Contributor

@EVODelavega Are you also planning for a documentation PR about that?

@EVODelavega
Copy link
ContributorAuthor

@soullivaneuh Just looking at the comments. I'd be more than happy to create a documentation PR, after I've fixed the docblocks.

@nicolas-grekas: Do I reallyhave to rebase + squash everything? If so: squash everything into a single commit and rebase on master?

@EVODelavega
Copy link
ContributorAuthor

@soullivaneuh Createddocumentation PR

Copy link
Member

Choose a reason for hiding this comment

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

This change should be done in a different PR.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Created a PR for the typo. Should I revert this change, or simply wait for the other PR to get merged and rebase the changes here?

@EVODelavegaEVODelavega mentioned this pull requestDec 21, 2015
fabpot added a commit that referenced this pull requestDec 21, 2015
This PR was merged into the 3.1-dev branch.Discussion----------Fix typo in changelog| Q             | A| ------------- | ---| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#17084| License       | MIT| Doc PR        | -Create separate PR as requested in [OptionsResolver PR](#17032)Commits-------6508b3d Fix typo in changelog

Choose a reason for hiding this comment

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

all the calls to mb_substr should be replaced by plain substr calls imho

if (function_exists($isFunction ='is_'.$type)) {
return$isFunction($value);
}

Copy link
Member

Choose a reason for hiding this comment

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

The parenthesis can be removed.

@fabpot
Copy link
Member

This looks like a good new feature. What do you think @symfony/deciders?

CHANGELOG
=========

3.1.0
Copy link
Member

Choose a reason for hiding this comment

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

Should be changed to 3.2.0 now.

@Tobion
Copy link
Contributor

Tobion commentedJun 16, 2016•
edited
Loading

In general I think the feature is not bad. I'm just not sure if it's worth since there is an easy solution already:#15524 (comment)

Also, the allowed types now look like phpdoc vars but do not actually support all of the phpdoc features likeint|float or(Foo|Bar)[] are not possible. So it's kinda arbitrary what it would support.

@fabpot
Copy link
Member

I don't find the solution you mentioned that easy; and probably less than just having to add[]. I like the fact that we are reusing phpdoc notation here, and the fact that we do not support the whole notation yet does mean that this is arbitrary.

@xabbuh
Copy link
Member

I am 👍 for supporting this. Looks quite reasonable and straight forward to me.

@Tobion
Copy link
Contributor

Tobion commentedJun 18, 2016•
edited
Loading

Keep in mind the phpdoc interpretation ofint[] implemented here does only support arrays of integers, not traversables of integers. This seems consistent with the current working state of PSR-5. For collections one would need to specifiy\Traversable<int> which is not implemented here.

@fabpot
Copy link
Member

@Tobion I think this is not relevant for this PR as we are not implementing a phpdoc or PSR-5. We are just reusing something that people can understand easily as they are familiar with this notation.

@fabpot
Copy link
Member

@EVODelavega Do you have time to take the comments into accounts?

@EVODelavega
Copy link
ContributorAuthor

@fabpot It's been a while since I last looked at this PR, but I'm going to take all of the comments into account, work on it some more, and I'll let you know when it's ready.

Thanks.

@fabpot
Copy link
Member

@EVODelavega Are you still interested in finishing this PR?

@nicolas-grekasnicolas-grekas added this to the3.x milestoneDec 6, 2016
@bendavies
Copy link
Contributor

Just came searching for this feature. Any interest in finishing this off? Or someone taking over?

@bendavies
Copy link
Contributor

I don't mind taking over if OP has no time.

@soullivaneuh
Copy link
Contributor

@EVODelavega Are you still interested in finishing this PR?

Or maybe someone else?

@pierredup
Copy link
Contributor

@soullivaneuh see#23112

@EVODelavega
Copy link
ContributorAuthor

Ouch, is this still going? Erm, I suppose I can pick it up again should this be needed. It looks like the other PR implements this feature, though. I've switched from PHP to golang since opening this PR, so if there's an alternative PR that is more up-to-date, feel free to close this one.

@nicolas-grekas
Copy link
Member

@EVODelavega would you be up rebasing this PR on 3.4 by the end of the week? Feature freeze is coming and there are some comments still pending also.

@nicolas-grekasnicolas-grekas modified the milestones:3.4,4.1Oct 8, 2017
fabpot added a commit that referenced this pull requestOct 12, 2017
…pe (pierredup)This PR was merged into the 3.4 branch.Discussion----------[OptionsResolver] Support array of types in allowed type| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#17032,#15524| License       | MIT| Doc PR        | TBDThis replaces#17032 with a simpler approach to allow an array of types in the allowed types for the options resolverNote: This implementation doesn't support nested values (I.E `int[][]`), but if there is a strong need for it, I'll add it in another PRCommits-------d066a23 Support array of types in allowed type
@fabpotfabpot closed thisOct 12, 2017
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestJan 2, 2018
…VODelavega, wouterj)This PR was merged into the 3.4 branch.Discussion----------[OptionsResolver] Documented validation of array typesThis continues#6048 . The feature was finally merged into symfony 3.4 (congratz@EVODelavega!), so let's document it!Related Symfony PR:symfony/symfony#17032Commits-------cb12592 Merged new example in older example, removing some text318e199 Fix typobf5d9d3 Document typed arrays in optionsresolver
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

12 participants

@EVODelavega@stof@nicolas-grekas@soullivaneuh@fabpot@Tobion@xabbuh@bendavies@pierredup@jakzal@javiereguiluz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp