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

Normalise codebase with PHP 5.6 features #6166

Merged
merged 8 commits into from Dec 12, 2016

Conversation

lcobucci
Copy link
Member

@lcobucci lcobucci commented Dec 8, 2016

Sorry for the HUGE pr here but IMO we need to do that so our contributors can have a better source for their tests (and we don't need to ask for these kind of stuff over and over again).

@lcobucci lcobucci self-assigned this Dec 8, 2016
@Majkl578
Copy link
Contributor

Majkl578 commented Dec 8, 2016

Hmm, I'd really like to see generators in action, especially in hydrators etc... 😉

@dennisdegreef
Copy link

Generators have the disadvantage of not being compatible with array_map and such (unfortunatly). Would love to see that if they would. They are there for not backwards compatible with arrays.

@Ocramius
Copy link
Member

Ocramius commented Dec 8, 2016

Patch seems good, but I'm reluctant to merge it due to all the merge conflicts it will cause. Also, develop is due a rebase again, and this will make it quite terrible...

@lcobucci
Copy link
Member Author

lcobucci commented Dec 8, 2016

@Ocramius I can work on the rebase on a different branch before the merge just to do a dry run

@Ocramius
Copy link
Member

Ocramius commented Dec 8, 2016

@lcobucci it would also be fine to merge this only into develop.

@lcobucci
Copy link
Member Author

lcobucci commented Dec 8, 2016

@Ocramius I'll come with some suggestions after I get home 😉

@Majkl578
Copy link
Contributor

Majkl578 commented Dec 8, 2016

Well, develop is 7.0+, how about also adding i.e. scalar typehints then?

@Ocramius
Copy link
Member

Ocramius commented Dec 8, 2016

@Majkl578 can be done, but needs accurate migration documentation with every signature modification

@Majkl578
Copy link
Contributor

Majkl578 commented Dec 8, 2016

@Ocramius Of course, the real pain would be return types in interfaces I guess because of their variance (parameters are fine in terms of migration).
After migrating a code from 7.0 to 7.1, I still feel like it should be 7.1 or nothing though, since the type declarations in 7.0 are half-baked and nullables & void make a huge difference...

@greg0ire
Copy link
Member

greg0ire commented Dec 8, 2016

If these modifications were done with some kind of script (php-cs-fixer?), wouldn't using git filter-branch the-script on develop prior to rebasing it make the rebase easy?

@lcobucci
Copy link
Member Author

lcobucci commented Dec 9, 2016

@Ocramius I've started to rebase develop (without these changes) and even with some broken tests I'm almost done. but the tests are broken functional tests are broken and we have conflicts when git tries to apply the very first commit. I'd say that, despite of this patch, we must first solve the test suite, organise the commits and then think about syncing with master.

I've created this PR to have a better baseline for contributing to the ORM. My idea was to have it on master because we have a long journey to achieve v3 and a lot of people will still create new test cases copying things we have.

I understand that it's a freaking huge amount of changes but I think that it needed to be done in order to keep our code standardised (as much as possible).

@lcobucci
Copy link
Member Author

@Ocramius we have develop synced with master on develop-synced-2016-12-09 and develop sync with this branch on develop-sync-with-cleanup.

All the conflicts were solved so there should be no concern regarding this now.

Would be good if you and @guilhermeblanco could check and decide about this clean-up 😉.

@Ocramius
Copy link
Member

@lcobucci given that the rebase worked, this can be merged, thanks!

@Ocramius Ocramius added this to the 2.6.0 milestone Dec 12, 2016
@Ocramius Ocramius merged commit 2437715 into doctrine:master Dec 12, 2016
@Ocramius
Copy link
Member

@lcobucci I force-pushed develop with develop-sync-with-cleanup (23e6967)

@lcobucci lcobucci deleted the use-php56-features branch December 12, 2016 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants