Skip to content

Conversation

@dunglas
Copy link
Member

@dunglas dunglas commented Jan 18, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

@paradajozsef
Copy link
Contributor

👍

One more thing to note. If such a yml is provided to the loader:

Acme\MyObj:
    attributes:
        foo:
            groups: 'group1'

Invalid argument supplied for foreach() warning raises, because it is not ensured here.

If you think this should be in another PR, than sorry. :)

@dunglas
Copy link
Member Author

dunglas commented Jan 19, 2016

Good catch @paradajozsef, I've added a separate commit in this PR.

@paradajozsef
Copy link
Contributor

👍

@GuilhemN
Copy link
Contributor

This is more a bug fix than a new feature ;-)

👍 debugging is easier with this kind of check 😄

@xabbuh
Copy link
Member

xabbuh commented Jan 19, 2016

We should include the name of the file and probably some other related information (class name, property or something like that) to make it easier to spot the place where the configuration is wrong.

@fabpot
Copy link
Member

fabpot commented Jan 25, 2016

I agree with @xabbuh

@dunglas
Copy link
Member Author

dunglas commented Jan 26, 2016

Status: Need Review

@xabbuh @fabpot done

@fabpot
Copy link
Member

fabpot commented Jan 26, 2016

Thank you @dunglas.

fabpot added a commit that referenced this pull request Jan 26, 2016
This PR was squashed before being merged into the 2.7 branch (closes #17430).

Discussion
----------

[Serializer] Ensure that groups are strings

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Commits
-------

0a3b877 [Serializer] Ensure that groups are strings
@fabpot fabpot closed this Jan 26, 2016
@dunglas dunglas deleted the sr_yaml_groups branch January 26, 2016 07:01
@fabpot fabpot mentioned this pull request Feb 3, 2016
This was referenced Feb 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants