Upgrade to Symfony 3.0 compatibility #45
Conversation
|
When does this PR merged ? |
|
@philippe-levan: ping, any news here? |
|
Hi, |
|
I'm not very familiar with travis at this point, but this is the only error message showing up. |
|
Never mind, I wasn't reading far enough. |
|
@philippe-levan Why do you git ignore composer.lock? I get exactly the same errors in master even though travis shows it as passing. I believe this is because some dependencies have updated since then. |
|
@geekdevs we have to ignore the composer.lock in bundles. It is the project that fixes some versions, not the included bundles. Best regards, |
|
The issue looks like it's how symfony 3.0 requires a string of the class name instead of the actual class when creating a form from the factory. I'll look into fixing it in the next few days unless someone else beats me to it. |
|
I solved |
|
@drewclauson it was merged, please rebase from master |
|
@drewclauson thanks for working on this issue. Keep in mind it would be better to stay backward compatible with ~2.1 versions if it is possible. Otherwise I will do a new major version for this upgrade. Thanks, |
|
@geekdevs with your PR #49 merged, tests are still failing due to an evolution of symfony/form 3.x. I is the issue that mentions @drewclauson in its comment 172937869. Regards, Philippe |
|
@philippe-levan yes, I know that. But still need to rebase as otherwise it would fail on that dbpath thing. |
|
@philippe-levan From my research and initial testing, we are going to have to release a new major version. I don't see a path for this to work once we hit 3.0. How would you like me to handle this? I'm working through the fixes today. |
8c3c38b to
8b30255
Compare
|
All tests are now passing and I removed PHP 5.3 and 5.4 from Travis since Symfony 3.0 is >=5.5.9. Also added in testing on PHP 7.0 for good measure. This is definitely going to need to be a new major version - @philippe-levan anything specific I need to do in order to help make that happen? |
|
Update: I think it'll need to be a new major version, as I don't see an easy way to make the form changes backwards compatible. Also, I did note that tests failed for me when I tried to run master against Symfony 2.1 or 2.2. I think the biggest issue was the namespace of |
|
@drewclauson According to this doc http://symfony.com/doc/current/contributing/community/releases.html#schedule I think we need to support at least 2.3, 2.7, 2.8 since these are LTS versions. Looking through your update it looks like the only major change is usage of class names for form elements instead of objects in form builder. I don't see it as being hard to support. Also, even though v.3 is PHP>=5.5.9, we can still allow lower versions of php for v.2 branch (need to setup travis accordingly). |
|
@geekdevs thanks for the info, I agree with supporting all current LTS. I'm not well experienced with travis configuration. What would that configuration look like? Does travis also have the ability to test against multiple symfony versions rather than just the latest one the composer.json specifies? |
|
@drewclauson correct. I made some experiments with it, see example config here https://github.com/geekdevs/TbbcMoneyBundle/blob/master/.travis.yml And that's how travis tests it https://travis-ci.org/geekdevs/TbbcMoneyBundle |
464cb8f to
43c99c7
Compare
|
Ok, all tests passing and this PR supports Symfony >=2.3. The readme says that it supports Symfony >= 2.1, but I'm not sure how I would make it support back to 2.1, due to the change in namespace on @geekdevs Thanks for your help and input. This has been a great learning experience for me! |
|
@drewclauson Thanks for updates. But looks like tests are failing now. Quick check shows that this is not related to your change, but it looks quite weird and should be fixed anyway.
|
|
I've created a PR for this |
|
@geekdevs I was just about to look into this and saw your message. Thanks! @philippe-levan Please merge #50 and then I'll rebase and that should make this PR ready to go. |
|
@drewclauson #50 merged. Thanks both (@geekdev and you) for your job on that bundle ! |
|
any news @drewclauson ? |
|
@geekdevs I've been under a lot of pressure at work to get a few deadlines done. I may not be able to revisit this until mid-Feb. :-( |
|
@geekdevs @drewclauson How is it ? Thanks :) |
|
@mrcmorales Unfortunately, I'm not able to return to it, but I just reviewed the last few notes and will follow up on a questions I have. @geekdevs How do we get phpunit-bridge into travis? Is that in a config? |
|
@geekdevs Never mind, I've only drank half my ☕ today so far... Just pushed a composer.json with phpunit-bridge included |
|
@drewclauson I've created a pull request, see #56 There are couple things why it did not show any errors (it will now):
|
|
@philippe-levan Please merge my PR so that we could continue testing symfony3 integration |
|
@drewclauson so my PR was merged, please rebase from master, set |
|
Unfortunately, my work was re-prioritized a few weeks ago and I'm no longer working on our backend transition, which requires this library (and no one else at our company is either until we do some legacy system upgrades) but am switching gears to our legacy system... I'll leave my fork available for someone to pick up, but I won't be able to revisit this for at least 4-5 months. 😦 |
|
@drewclauson normally people work on open source projects in their spare time ;). I can continue with it, no problem. Just thought you'd want to finish up yourself. |
|
@geekdevs I know, I unfortunately, don't have the spare time most are able to commit to open source projects and have only been able to get more involved lately due to needing this for my job. 😐 I wish I did have the time to finish this up. I think you can just fork my fork and create a PR back into this repo, but I haven't tried that before. Thank you and I do apologize that I can't work on it further at this point. |
|
@drewclauson that's ok. You'd done a lot there already, that's much appreciated. |
|
@drewclauson i do agree with @geekdevs. Thanks for your contributions ! (and I'm desperately not available neither...) |
|
Is there anything left blocking this PR? or is it only a travis config issue? |
|
@wavyx it has a lot of backwards compatibility issues. Travis config fix would basically just show them in tests. Nothing crazy complicated, but needs time to solve. |
|
Any downside to removing backwards compatibility and tagging this as a new major version? |
|
@gregholland A new major version is needed to go to SF3. But it still needs to be green in travis. But right now it is not compatible with SF3 and I have absolutely no time for working on this adaptation. (and I don't have any project on SF3 right now). This PR is the most advanced adaptation right now (thanks @geekdevs and @drewclauson for that). |
|
@philippe-levan if you create a branch for SF3 we can go start testing and debugging and make PR's to solve any problem. Can you do that? btw, A big downside of this repository is that it contains a library and bundle in one.
|
|
As far as I can tell there is no big problem for making this bundle backwards compatible, so I would vote for keeping it in minor release. So if any of you guys have time to finish it faster you can fork @drewclauson's branch and work from there |
|
@geekdevs why not just merge it, and solve issue by issue. thats way more easier then fix it all right? |
|
I am good with that approach. If @philippe-levan creates feature branch for that we could merge @drewclauson's fork and start fixing from there. |
|
Hi @geekdevs, @verschoof, I merged this PR on a public branch : "symfony3-migration" https://github.com/TheBigBrainsCompany/TbbcMoneyBundle/tree/symfony3-migration This feature branch will be merged on master. Master is now the branch for the symfony3 version of the bundle and I created a 2.x branch for the symfony 2.x version. Do you need something else from me ? Best regards, PS : I modified .travis-ci and composer.json on the symfony3-migration branch to be compatible only with sf3 and not sf2 anymore. Only the branch 2.x remains compatible with sf2. |
|
hi @philippe-levan , Is there some version of this bundle with money v3 compatible with sf 2.7 or 2.8 ? Thanks |
|
@mrcmorales : This comment should be another issue. It is not the same problem. I understand your are talking of the v3 of the moneyphp/money (https://github.com/moneyphp/money/tree/v3.0.0-beta) right ? No, we won't support moneyphp/money:~3 in that bundle. There is too much BC Breaks in the library (and one reason to remain on the v2). We can't remain stable enough in our interfaces. I believe it would be a better idea to create another bundle for that moneyphp/money:~3 version. Best regards, |
|
closed following the comment : #45 (comment) |
|
I've added a PR that should fix Symfony 3 integration That should support both 2.8 and 3.0 |
For #44
I have tested this in my environment and haven't gotten any deprecation warnings or error messages with Symfony 2.8 or 3.0. I don't have a ton of background with upgrading bundles, but it seems to me like it'll work just by updating the dependencies.