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

[Serializer] Add default object class resolver #31026

Merged
merged 1 commit into from Apr 10, 2019

Conversation

Projects
None yet
6 participants
@jdecool
Copy link
Contributor

jdecool commented Apr 8, 2019

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

The commit 1d8b5af introduce a BC break because before that commit the extractAttributes the $object can be a string which contain the fully qualified name of an object.

To fix the BC break and preserve the new feature, I suggest to create a default object class resolver if it is not set by the developer.

@joelwurtz

This comment has been minimized.

Copy link
Contributor

joelwurtz commented Apr 9, 2019

Do you have some case where the object is a class and not an object ?

I'm not sure we want to do that, as the current signature https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php#L237 specify an object (and not a string or object).

@jdecool

This comment has been minimized.

Copy link
Contributor Author

jdecool commented Apr 9, 2019

I have the problem on a project I'm working on.

I know that the interface specify that the argument need to be an object.

But the class isn't specify as @internal and therefore can be used by inheritance. The argument cannot by strongly type in the method because of PHP requirements.

So it create an implicit contract for existing apps. And the change can break some projects whereas we want to make a bugfix update of Symfony.

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Apr 9, 2019

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

(for 4.2)

@nicolas-grekas nicolas-grekas removed the BC Break label Apr 9, 2019

@joelwurtz

This comment has been minimized.

Copy link
Contributor

joelwurtz commented Apr 9, 2019

can we maybe trigger an error / deprecation when it's not an object and make it strict in 5.0 (by throwing an error if not an object), not necessarily here, will do the PR if that's ok with you.

@jdecool

This comment has been minimized.

Copy link
Contributor Author

jdecool commented Apr 9, 2019

I push the changed suggested by @nicolas-grekas.

@joelwurtz I let you trigger the error, it's ok for me

@joelwurtz

This comment has been minimized.

Copy link
Contributor

joelwurtz commented Apr 9, 2019

@jdecool like @nicolas-grekas said you should base your branch on 4.2 and not on master (otherwise you will still have the bug in new 4.2 versions :) )

@jdecool jdecool changed the base branch from master to 4.2 Apr 9, 2019

@jdecool

This comment has been minimized.

Copy link
Contributor Author

jdecool commented Apr 9, 2019

@joelwurtz Oh sorry.

It's now OK 👍

@fabpot

fabpot approved these changes Apr 10, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Apr 10, 2019

Thank you @jdecool.

@fabpot fabpot merged commit dd5b8f1 into symfony:4.2 Apr 10, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 10, 2019

bug #31026 [Serializer] Add default object class resolver (jdecool)
This PR was squashed before being merged into the 4.2 branch (closes #31026).

Discussion
----------

[Serializer] Add default object class resolver

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

The commit 1d8b5af introduce a BC break because before that commit the `extractAttributes` the `$object` can be a string which contain the fully qualified name of an object.

To fix the BC break and preserve the new feature, I suggest to create a default object class resolver if it is not set by the developer.

Commits
-------

dd5b8f1 [Serializer] Add default object class resolver

@fabpot fabpot referenced this pull request Apr 16, 2019

Merged

Release v4.2.6 #31125

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.