Skip to content

Conversation

@tgalopin
Copy link
Contributor

@tgalopin tgalopin commented Sep 3, 2016

Based on #288 for the coding style improvements and especially on #283. Thanks @DenisMedved, @dfridrich, @bassrock and @ubermuda for your awesome work!

This PR adds support for Symfony 3 by implementing non-BC-break new features and implementing a a BC layer for the other. Any test would be awesome :) ! I tried with a demo project and it's working fine, but feel free to try it more extensively :) !

@lsmith77 lsmith77 added the wip/poc Work in progress or proof of concept label Sep 3, 2016
@tgalopin tgalopin force-pushed the support-sf3 branch 4 times, most recently from 5b52ce8 to ac6d06a Compare September 3, 2016 14:44
- php: 5.3
env:
- deps=low
- SYMFONY_VERSION=2.7.*
Copy link
Member

Choose a reason for hiding this comment

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

This looks useless and might hide forgotten dependencies, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain a bit more your idea? I don't understand why it would hide dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Because you are always loading symfony entirely and you might depend on symfony libraries not included in the real composer file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are requiring the framework bundle, not Symfony entirely, and we do require the framework bundle in the composer.json.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed sorry, i'm used to libraries loading the entire framework...
That's fine for me then 👍

@tgalopin
Copy link
Contributor Author

tgalopin commented Sep 4, 2016

Given the BC break we have for the controller (and the potential other BC breaks we will get in the future), I suggest we adopt the same principles than Symfony:

  • in the 1.2 branch, I merge this PR (when ready ofc) without the BC break (keeping extends ContainerAware): this way, the depreciations are introduced in 1.2 and people can upgrade smoothly ;
  • in master, I introduce the BC breaks (I also remove the deprecated calls) and put all this in the UPGRADE file ;
  • in the future, we keep the same system: 3.0 will be 2.X without deprecated code, etc.

What do you think?

@GuilhemN
Copy link
Member

GuilhemN commented Sep 4, 2016

Imo this belongs to a minor version (that's not a bug fix) so i would merge everything in master.
For majors, symfony's policy is good but i don't think we need to be as strict for small changes that need complex bc layers.

@tgalopin
Copy link
Contributor Author

tgalopin commented Sep 4, 2016

I tend to agree. After all, relying on the extends is quite specific and shouldn't be a problem.

I still would like to set master as 2.0-dev though (by putting all the depreciations introduced in this PR in the 1.2 branch instead of master). Do you think it would be a problem?

@GuilhemN
Copy link
Member

GuilhemN commented Sep 4, 2016

@tgalopin i don't think there are enough deprecations to release 2.0 (moreover they are not locking new features) so imo master should target 1.3.
Unless you have plans for the future?

@tgalopin
Copy link
Contributor Author

tgalopin commented Sep 4, 2016

Keep in mind there are technically 88 commits in 1.3 then :) (https://github.com/FriendsOfSymfony/FOSMessageBundle/tree/1.2). Last release of 1.2 was 3 years ago :/ .

@GuilhemN
Copy link
Member

GuilhemN commented Sep 4, 2016

Is it a problem if they are all bc?

@tgalopin
Copy link
Contributor Author

tgalopin commented Sep 4, 2016

No, technically not. I will merge in master then.

@GuilhemN
Copy link
Member

GuilhemN commented Sep 4, 2016

For example we released fosrestbundle 2.0 because we wanted many features requiring bc breaks.
But you're the maintainer of this bundle, maintenance cost is also a good reason for releasing a major version so if you think maintaining legacy code is too complicated you can of course release 2.0.

@tgalopin tgalopin force-pushed the support-sf3 branch 3 times, most recently from 2682c1e to 6719a8f Compare September 4, 2016 18:32
@tgalopin
Copy link
Contributor Author

tgalopin commented Sep 4, 2016

What I had in mind is not related to which release we choose (I was going to release 1.3 anyway), simply I wanted to start to prepare master as 2.0 by removing the depreciation from it.

I updated the PR, don't hesitate to recheck :) .

@GuilhemN
Copy link
Member

GuilhemN commented Sep 4, 2016

If you don't plan to release 2.0 soon it will only add maintenance cost (you'll need to merge 1.3 in master each time your merge a pr), that's why sf didn't create a 4.0 branch btw.

* Sets the container.
*
* @param ContainerInterface|null $container A ContainerInterface instance or null
*/
Copy link
Member

Choose a reason for hiding this comment

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

This doc bloc brings no value imo

@GuilhemN
Copy link
Member

GuilhemN commented Sep 4, 2016

Thanks for the great work ! That's a big task...

@tgalopin
Copy link
Contributor Author

tgalopin commented Sep 4, 2016

If you don't plan to release 2.0 soon it will only add maintenance cost (you'll need to merge 1.3 in master each time your merge a pr), that's why sf didn't create a 4.0 branch btw.

That's true. I will see how the releases evolve and create a 2.0 if enough deprecations will be removable.

Thanks for the review, I'm fixing the comments.

@tgalopin
Copy link
Contributor Author

tgalopin commented Sep 4, 2016

Updated. I think this is mergeable (I'll dedicate a PR for CS).

@tgalopin tgalopin merged commit 4e6c19e into master Sep 7, 2016
@lsmith77 lsmith77 removed the wip/poc Work in progress or proof of concept label Sep 7, 2016
@tgalopin tgalopin deleted the support-sf3 branch October 19, 2016 12:15
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.

4 participants