[OptionsResolver] support array of instance validation #17032

EVODelavega · 2015-12-16T15:24:52Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#15524
License	MIT
Doc PR	symfony/symfony-docs#6048

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

Closing PR 15671.

EVODelavega · 2015-12-16T16:23:59Z

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

xabbuh · 2015-12-17T21:43:32Z

src/Symfony/Component/OptionsResolver/OptionsResolver.php

+     * @param string $type  the required allowedType string
+     * @param mixed  $value the value
+     *
+     * @return bool Whether or not $value if of the allowed type


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

stof · 2015-12-17T21:59:20Z

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

nicolas-grekas · 2015-12-18T09:19:17Z

Status: needs work (git history rebasing+squashing)

soullivaneuh · 2015-12-18T10:19:28Z

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

EVODelavega · 2015-12-18T10:54:55Z

@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 really have to rebase + squash everything? If so: squash everything into a single commit and rebase on master?

EVODelavega · 2015-12-18T17:57:29Z

@soullivaneuh Created documentation PR

xabbuh · 2015-12-21T08:49:27Z

CHANGELOG-3.0.md

@@ -26,7 +26,7 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
 * bug #16312 [HttpKernel] clearstatcache() so the Cache sees when a .lck file has been released (mpdude)
 * bug #16351 [WIP] [Form] [TwigBridge] Bootstrap horizontal theme missing tests (pieter2627)
 * feature #16715 [Form] Remove choices_as_values option on ChoiceType (nicolas-grekas)
- * feature #16692 [Form] Drop remaing CsrfProviderAdapter/Interface mentions (nicolas-grekas)
+ * feature #16692 [Form] Drop remaining CsrfProviderAdapter/Interface mentions (nicolas-grekas)


This change should be done in a different PR.

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?

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

nicolas-grekas · 2015-12-28T12:49:09Z

src/Symfony/Component/OptionsResolver/OptionsResolver.php

+     */
+    private function verifyAllowedType($type, $value)
+    {
+        if (mb_substr($type, -2) === '[]') {


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

fabpot · 2016-06-16T19:45:09Z

src/Symfony/Component/OptionsResolver/OptionsResolver.php

+            return $isFunction($value);
+        }
+
+        return ($value instanceof $type);


The parenthesis can be removed.

fabpot · 2016-06-16T19:46:33Z

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

fabpot · 2016-06-16T19:46:51Z

src/Symfony/Component/OptionsResolver/CHANGELOG.md

@@ -1,6 +1,11 @@
 CHANGELOG
 =========

+3.1.0


Should be changed to 3.2.0 now.

Tobion · 2016-06-16T21:53:03Z

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 like int|float or (Foo|Bar)[] are not possible. So it's kinda arbitrary what it would support.

fabpot · 2016-06-17T05:34:26Z

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 · 2016-06-17T06:43:42Z

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

Tobion · 2016-06-18T21:49:18Z

Keep in mind the phpdoc interpretation of int[] 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 · 2016-06-19T10:23:23Z

@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 · 2016-06-21T14:26:26Z

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

EVODelavega · 2016-06-21T16:44:48Z

@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 · 2016-11-09T23:39:10Z

@EVODelavega Are you still interested in finishing this PR?

bendavies · 2017-03-06T21:41:02Z

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

bendavies · 2017-03-06T21:50:24Z

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

soullivaneuh · 2017-06-19T16:27:33Z

@EVODelavega Are you still interested in finishing this PR?

Or maybe someone else?

pierredup · 2017-06-20T04:19:35Z

@soullivaneuh see #23112

EVODelavega · 2017-06-23T12:33:51Z

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 · 2017-09-26T12:10:43Z

@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.

…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 | TBD This replaces #17032 with a simpler approach to allow an array of types in the allowed types for the options resolver Note: 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 PR Commits ------- d066a23 Support array of types in allowed type

@EVODelavega

…VODelavega, wouterj) This PR was merged into the 3.4 branch. Discussion ---------- [OptionsResolver] Documented validation of array types This continues #6048 . The feature was finally merged into symfony 3.4 (congratz @EVODelavega!), so let's document it! Related Symfony PR: symfony/symfony#17032 Commits ------- cb12592 Merged new example in older example, removing some text 318e199 Fix typo bf5d9d3 Document typed arrays in optionsresolver

carsonbot added the Status: Needs Review label Dec 16, 2015

EVODelavega mentioned this pull request Dec 16, 2015

[OptionsResolver] support array of instance validaion #15671

Closed

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 16, 2015

xabbuh reviewed Dec 17, 2015
View reviewed changes

nicolas-grekas added Status: Needs Work and removed Status: Reviewed labels Dec 18, 2015

EVODelavega force-pushed the feature/issue-15524 branch from 3aa9e04 to 7b0594d Compare December 18, 2015 17:39

EVODelavega mentioned this pull request Dec 18, 2015

Document typed arrays in optionsresolver symfony/symfony-docs#6048

Closed

xabbuh reviewed Dec 21, 2015
View reviewed changes

EVODelavega mentioned this pull request Dec 21, 2015

Fix typo in changelog #17083

Merged

Elias Van Ootegem and others added 12 commits December 21, 2015 11:42

Suggested fix for issue symfony#15524

d4de104

Fix style issues (apply patch)

82f825c

Remove unused variable from method call

c42c1f4

Fix recursive call + add simple testcase

3609d44

Fix coding style

6b1c230

Add 2 successful testcases

0194c25

Fix coding style

ca21329

Conform to coding standards (PSR-2 => elseif)

6b639cf

Add early returns in response to review

8ff6aac

Add whitespace before return statement

4ab68e0

Format complex types in exception message, too

2685d2e

Fix coding style

1fc5ddb

nicolas-grekas reviewed Dec 28, 2015
View reviewed changes

javiereguiluz added Feature OptionsResolver labels Mar 3, 2016

fabpot reviewed Jun 16, 2016
View reviewed changes

src/Symfony/Component/OptionsResolver/CHANGELOG.md

@@ -1,6 +1,11 @@

CHANGELOG

=========

3.1.0

Copy link

Member

fabpot Jun 16, 2016

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.

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

pierredup mentioned this pull request Jun 8, 2017

[OptionsResolver] Support array of types in allowed type #23112

Merged

nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

fabpot closed this Oct 12, 2017

wouterj mentioned this pull request Dec 31, 2017

[OptionsResolver] Documented validation of array types symfony/symfony-docs#8946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OptionsResolver] support array of instance validation #17032

[OptionsResolver] support array of instance validation #17032

EVODelavega commented Dec 16, 2015

EVODelavega commented Dec 16, 2015

xabbuh Dec 17, 2015

jakzal Dec 17, 2015

stof commented Dec 17, 2015

nicolas-grekas commented Dec 18, 2015

soullivaneuh commented Dec 18, 2015

EVODelavega commented Dec 18, 2015

EVODelavega commented Dec 18, 2015

xabbuh Dec 21, 2015

EVODelavega Dec 21, 2015

nicolas-grekas Dec 28, 2015

fabpot Jun 16, 2016

fabpot commented Jun 16, 2016

fabpot Jun 16, 2016

Tobion commented Jun 16, 2016 •

edited

Loading

fabpot commented Jun 17, 2016

xabbuh commented Jun 17, 2016

Tobion commented Jun 18, 2016 •

edited

Loading

fabpot commented Jun 19, 2016

fabpot commented Jun 21, 2016

EVODelavega commented Jun 21, 2016

fabpot commented Nov 9, 2016

bendavies commented Mar 6, 2017

bendavies commented Mar 6, 2017

soullivaneuh commented Jun 19, 2017

pierredup commented Jun 20, 2017

EVODelavega commented Jun 23, 2017

nicolas-grekas commented Sep 26, 2017

[OptionsResolver] support array of instance validation #17032

[OptionsResolver] support array of instance validation #17032

Conversation

EVODelavega commented Dec 16, 2015

EVODelavega commented Dec 16, 2015

xabbuh Dec 17, 2015

Choose a reason for hiding this comment

jakzal Dec 17, 2015

Choose a reason for hiding this comment

stof commented Dec 17, 2015

nicolas-grekas commented Dec 18, 2015

soullivaneuh commented Dec 18, 2015

EVODelavega commented Dec 18, 2015

EVODelavega commented Dec 18, 2015

xabbuh Dec 21, 2015

Choose a reason for hiding this comment

EVODelavega Dec 21, 2015

Choose a reason for hiding this comment

nicolas-grekas Dec 28, 2015

Choose a reason for hiding this comment

fabpot Jun 16, 2016

Choose a reason for hiding this comment

fabpot commented Jun 16, 2016

fabpot Jun 16, 2016

Choose a reason for hiding this comment

Tobion commented Jun 16, 2016 • edited Loading

fabpot commented Jun 17, 2016

xabbuh commented Jun 17, 2016

Tobion commented Jun 18, 2016 • edited Loading

fabpot commented Jun 19, 2016

fabpot commented Jun 21, 2016

EVODelavega commented Jun 21, 2016

fabpot commented Nov 9, 2016

bendavies commented Mar 6, 2017

bendavies commented Mar 6, 2017

soullivaneuh commented Jun 19, 2017

pierredup commented Jun 20, 2017

EVODelavega commented Jun 23, 2017

nicolas-grekas commented Sep 26, 2017

Tobion commented Jun 16, 2016 •

edited

Loading

Tobion commented Jun 18, 2016 •

edited

Loading