Skip to content

Conversation

@goetas
Copy link
Member

@goetas goetas commented Jun 14, 2018

This introduces the changes for supporting the upcoming jms/serializer 2.0 release

Some changes in composer.json and travisci are will not be necessary when serializer v1.13 will be released

@goetas goetas force-pushed the serializer-2.0-compat branch 2 times, most recently from bf5ea97 to dcf3fd8 Compare June 14, 2018 15:46
@goetas goetas force-pushed the serializer-2.0-compat branch 3 times, most recently from 751ac96 to 397080a Compare July 25, 2018 15:15
@goetas
Copy link
Member Author

goetas commented Jul 25, 2018

Tests failure is not related to this PR but to #1912

@goetas goetas force-pushed the serializer-2.0-compat branch from 397080a to 3429bc5 Compare July 25, 2018 15:17
@GuilhemN
Copy link
Member

I don't truly know jms internals so I can hardly review this, please tell me when you think it is ready to merge :)

@magnetik
Copy link
Contributor

I just faced it today so I'll hope it will get released :)

@goetas
Copy link
Member Author

goetas commented Sep 17, 2018

@magnetik

I just faced it today so I'll hope it will get released :)

Faced what? few days ago a first beta has been released (https://www.goetas.com/blog/whats-new-in-jmsserializer-v20/)

@magnetik
Copy link
Contributor

The current incompatibility between FOSRestBundle and JMSSerializerBundle 3.0

@goetas
Copy link
Member Author

goetas commented Sep 17, 2018

@magnetik The status of this PR is almost definitive, I do not expect major changes.

If you want to try JMSSerializerBundle 3.0 with jms/serializer 2.0 (that is beta for now) you can use my fork.

Something as:

{
   "require": {
       "friendsofsymfony/rest-bundle": "dev-serializer-2.0-compat as 2.3.99",
       "jms/serializer": "^2.0@beta",
       "jms/metadata": "^2.0@beta"
    }
    "repositories": [
        {
            "type": "vcs",
            "url": "[email protected]:goetas/FOSRestBundle.git"
        }
    ]
}

@goetas
Copy link
Member Author

goetas commented Sep 17, 2018

@GuilhemN I think that this should be merged when jms will enter in the release-candidate stage (in 3 weeks)

@GuilhemN
Copy link
Member

Ok, no problem, I'll merge it ☺

@goetas goetas force-pushed the serializer-2.0-compat branch 2 times, most recently from ed74e10 to 2b3d35d Compare November 9, 2018 15:40
@goetas goetas changed the title [WIP] jms/serializer 2.0 compatibility jms/serializer 2.0 compatibility Nov 9, 2018
@goetas goetas force-pushed the serializer-2.0-compat branch from 2b3d35d to 8201895 Compare November 9, 2018 15:48
@goetas
Copy link
Member Author

goetas commented Nov 9, 2018

This is ready for review now!

@goetas
Copy link
Member Author

goetas commented Nov 9, 2018

Not sure if that custom fos type handler makes sense. It should have huge performance impact. Is there a reason for it?

@GuilhemN
Copy link
Member

Not sure if that custom fos type handler makes sense. It should have huge performance impact. Is there a reason for it?

This is because of the Exception controller, we wanted users to be able to overide exceptions rendering for each sub class. I didn't find a better and simpler alternative at the time at least.

@goetas
Copy link
Member Author

goetas commented Nov 13, 2018

@GuilhemN I think the best approach is to keep this as it is and optimize the handler in a different PR.
IMO this is ready for rewiew or merge if it looks OK to you

if (null !== $handler) {
return $handler;
}
} while ($typeName = get_parent_class($typeName));
Copy link
Member Author

Choose a reason for hiding this comment

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

As this was used for the exception handler, will it make sense to limit this behavior only for classes extending Exception ?

Performance of this loop are really impacting serialization performance...

Copy link
Member

Choose a reason for hiding this comment

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

Sure we could do that indeed

@notrix
Copy link

notrix commented Nov 15, 2018

👍

@Saracevas
Copy link

Excellent work everyone, do we know when this will be merged in?

@GuilhemN GuilhemN merged commit 35ccb78 into FriendsOfSymfony:master Nov 25, 2018
@GuilhemN
Copy link
Member

Thank you @goetas :)

@goetas
Copy link
Member Author

goetas commented Nov 25, 2018

yay!

@goetas goetas deleted the serializer-2.0-compat branch November 25, 2018 08:26
@mansartesteban
Copy link

Hey guys !
Sorry but i don't understand i'm new to github community and the system of issues is new to me (and awesome by the way).
If i understand weel, you fixed this issue in commit 35ccb78 and pushed it to the master branch isn't it ?
So if the fix is on master and if i "composer friendsofsymfony/rest-bundle && composer update jms/serializer-bundle" it should be working no ?
I still have the same problem and the files JMSHandlerRegistry at me is the same than v2.3 ... and my composer info say that I have the v2.4.
I don't understand could someone help me plz :D !

@notrix
Copy link

notrix commented Nov 29, 2018

@mansartesteban make sure you have "friendsofsymfony/rest-bundle": "dev-master" in your composer.json if you want changes from master branch. Or you can wait till stable version release.

@mansartesteban
Copy link

Oh ! It works ! Thank you very much <3

xabbuh added a commit that referenced this pull request Nov 30, 2018
This PR was merged into the 2.3-dev branch.

Discussion
----------

Drop jms/serializer <1.13

~~`GraphNavigatorInterface` has been introduced in JMS Serializer v1.12.0. We should use `GraphNavigator` instead to maintain backward compatibility with v1.11.0 and lower.~~

#1899 breaks compatibility with `jms/serializer <1.13.0` by using:

- `JMS\Serializer\GraphNavigatorInterface` (>=1.12)
- `JMS\Serializer\Context::hasAttribute()` (>=1.13)
- `JMS\Serializer\Context::getAttribute()` (>=1.13)

However, it is still possible to install older version of `jms/serializer` together with FOSRestBundle. This PR fixes it by updating a `conflict` constraint.

Commits
-------

535e366 drop jms/serializer <1.13.0
@magnetik
Copy link
Contributor

magnetik commented Dec 4, 2018

Can a release be tagged with this fix?

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants