Skip to content

[FrameworkBundle][Serializer] Move SerializerPass to the Serializer #21293

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
Feb 16, 2017
Merged

[FrameworkBundle][Serializer] Move SerializerPass to the Serializer #21293

merged 1 commit into from
Feb 16, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Jan 14, 2017

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

Part of #21284

@chalasr chalasr mentioned this pull request Jan 14, 2017
Closed
11 tasks
@chalasr chalasr force-pushed the serializer/serializerpass branch 5 times, most recently from 88a93f4 to a328e75 Compare January 15, 2017 15:53
@nicolas-grekas nicolas-grekas added this to the 3.x milestone Jan 16, 2017
dunglas
dunglas reviewed Jan 16, 2017
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/composer.json
"symfony/translation": "~2.8|~3.0",
"symfony/templating": "~2.8|~3.0",
"symfony/validator": "~3.2",
"symfony/yaml": "~3.2",
"symfony/property-info": "~2.8|~3.0",
"symfony/property-info": "~3.1",
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we change the framework's serializer requirement to 3.3, and the serializer in 3.3 requires this. The build with low deps was failing otherwise:

  1. Symfony\Bundle\FrameworkBundle\Tests\Functional\SerializerTest::testDeserializeArrayOfObject
    Error: Class 'phpDocumentor\Reflection\FileReflector' not found

See https://travis-ci.org/symfony/symfony/jobs/191965990

Copy link
Member

Choose a reason for hiding this comment

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

Well, then it means that the serializer component should have the proper requirement or the proper conflict in place

Copy link
Member

Choose a reason for hiding this comment

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

It should be a conflict because the Serializer doesn't require PropertyInfo.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #21309

Copy link
Member

Choose a reason for hiding this comment

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

I'll open a PR for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dunglas done in #21309

Copy link
Member Author

Choose a reason for hiding this comment

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

reverted thanks to #21309

This was referenced Jan 16, 2017
Merged
Closed
@chalasr chalasr force-pushed the serializer/serializerpass branch from a328e75 to 9d3d592 Compare January 24, 2017 09:22
nicolas-grekas added a commit that referenced this pull request Jan 24, 2017
@nicolas-grekas
minor #21309 [Serializer] Add missing conflict for property-info<3.1 …
dd42d72 
…(chalasr)

This PR was merged into the 3.1 branch.

Discussion
----------

[Serializer] Add missing conflict for property-info<3.1

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21293 (comment)
| License       | MIT
| Doc PR        | n/a

Commits
-------

60a0c4b [Serializer] Add missing conflict for property-info<3.1
symfony-splitter pushed a commit to symfony/serializer that referenced this pull request Jan 24, 2017
@nicolas-grekas
minor #21309 [Serializer] Add missing conflict for property-info<3.1 …
f6c70e2 
…(chalasr)

This PR was merged into the 3.1 branch.

Discussion
----------

[Serializer] Add missing conflict for property-info<3.1

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/symfony#21293 (comment)
| License       | MIT
| Doc PR        | n/a

Commits
-------

60a0c4b [Serializer] Add missing conflict for property-info<3.1
@chalasr chalasr force-pushed the serializer/serializerpass branch 2 times, most recently from c6169c3 to fea6cd4 Compare January 24, 2017 14:22
@chalasr
Copy link
Member Author

chalasr commented Jan 24, 2017

@nicolas-grekas Is there any reason for adding this to the 3.x milestone but #21375 to the 3.3 one? They are about fixing the same ticket so if there is something wrong to you here, please tell me :)

@nicolas-grekas nicolas-grekas modified the milestones: 3.3, 3.x Jan 24, 2017
@chalasr chalasr force-pushed the serializer/serializerpass branch from fea6cd4 to e7e5ac9 Compare January 24, 2017 15:00
@chalasr
Copy link
Member Author

chalasr commented Jan 24, 2017

Updated this to make the serializer id and encoder/normalizer tags configurable through the constructor, defaulting to the ones used in the framework.

@chalasr chalasr force-pushed the serializer/serializerpass branch from e7e5ac9 to 5e54135 Compare January 24, 2017 15:09
@chalasr chalasr force-pushed the serializer/serializerpass branch from 5e54135 to 95cf508 Compare January 25, 2017 12:54
@chalasr
Copy link
Member Author

chalasr commented Jan 26, 2017 •
edited
Loading

But, isn't a BC break to add a parent class with a new constructor to the deprecated pass?
Edit: according to the BC promise:

Add constructor without mandatory arguments: Yes [1]
[1] Should be avoided. When done, this change must be documented in the UPGRADE file.

I guess it's fine here.

@chalasr
Copy link
Member Author

chalasr commented Feb 7, 2017

Do we want to move forward on this for 3.3? It would be nice to have a thought, to know if it's worth working on other relevant passes listed in the ticket using the same approach.

@xabbuh
Copy link
Member

xabbuh commented Feb 8, 2017

IMO it makes sense to move them.

@dunglas
Copy link
Member

dunglas commented Feb 8, 2017

👍

@fabpot
Copy link
Member

fabpot commented Feb 16, 2017

Thank you @chalasr.

@fabpot fabpot merged commit 95cf508 into symfony:master Feb 16, 2017
fabpot added a commit that referenced this pull request Feb 16, 2017
@fabpot
feature #21293 [FrameworkBundle][Serializer] Move SerializerPass to t…
846d24a 
…he Serializer (chalasr)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[FrameworkBundle][Serializer] Move SerializerPass to the Serializer

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

Part of #21284

Commits
-------

95cf508 [FrameworkBundle][Serializer] Move SerializerPass to the Serializer
@chalasr chalasr deleted the serializer/serializerpass branch February 16, 2017 14:18
@fabpot fabpot mentioned this pull request May 1, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas left review comments

@stof stof stof left review comments

Assignees
No one assigned
Labels
Deprecation Feature FrameworkBundle Serializer Status: Needs Review
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
7 participants
@chalasr @xabbuh @dunglas @fabpot @stof @nicolas-grekas @carsonbot