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

[Do not merge] Revert merge master into 2.2 #1923

Closed

Conversation

teohhanhui
Copy link
Contributor

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1850 (comment)
License MIT
Doc PR N/A

Relevant merged PRs (that should actually go into 2.2) since #1850:

d34fba5...2.2
(start from after d611aa0 and 3fdd546 which was when #1850 was merged)

This is done with git cherry-pick, so it's not meant for merging. If everything looks good, I will use this to create a revert commit and open another PR.

Unfortunately for @soyuka, you'll probably have to resubmit your 2 PRs if they're appropriate for 2.2. (Unless they can be rebased here.) Do you think that's okay?

meyerbaptiste and others added 24 commits May 2, 2018 15:03
…ow_dependencies

Fix the Behat test suite with low dependencies
Add missing doctrine alias removed in doctrine-bundle@1.9
Enable item route on collection subresources
The OrderFilter is still triggering bad deprecation/function calls
* Allow GraphQL to filter on nested property

fixes api-platform#1714, api-platform#1867

* Allow ordering on nested property values
@sroze
Copy link
Contributor

sroze commented May 2, 2018

Seems better than my approach of git revert -m 1 d611aa0991c90ca36d90318eff3f689eb3a98e5a and then manually fixing the conflicts:
https://github.com/api-platform/core/compare/2.2...sroze:2.2-but-better?expand=1

@teohhanhui
Copy link
Contributor Author

@sroze I told you so 😛

@sroze
Copy link
Contributor

sroze commented May 2, 2018

We had to try both ways 🤷‍♂️

@soyuka
Copy link
Member

soyuka commented May 2, 2018

Nice work ❤️

Could you quickly explain what you did? (I'm assuming reset [bad_sha1]~1 and cherry-pick correct ranges?)

Unfortunately for @soyuka, you'll probably have to resubmit your 2 PRs if they're appropriate for 2.2. (Unless they can be rebased here.) Do you think that's okay?

Which ones? Yeah it's okay I just need to make sure I have the work locally somewhere :P.

This is done with git cherry-pick, so it's not meant for merging. If everything looks good, I will use this to create a revert commit and open another PR.

Smart move although we could force push 😈

@teohhanhui
Copy link
Contributor Author

@soyuka See the checklist above. And you can rebase them based on the old commits in this repo. We're not going to rebase. This is only how I come up with the manual revert commit. =P

@soyuka
Copy link
Member

soyuka commented May 2, 2018

Are both fixes to the 2.2 branch.

Only #1899 should've been merged on master instead of 2.2.

@teohhanhui
Copy link
Contributor Author

Travis is passing: https://travis-ci.org/teohhanhui/api-platform-core/builds/373994838

The merge conflicts here are to be ignored. 😄

@sroze
Copy link
Contributor

sroze commented May 2, 2018

Yeah it's okay I just need to make sure I have the work locally somewhere :P.

@soyuka I'm happy to help you re-submit #1899, I'm pretty knowledgeable about last 20 days changes now xD

Copy link
Contributor

@sroze sroze left a comment

Choose a reason for hiding this comment

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

Let's have one revert commit instead. I know you have it ready @teohhanhui, thank you ❤️

@sroze
Copy link
Contributor

sroze commented May 2, 2018

Closed in favor of #1924

@sroze sroze closed this May 2, 2018
@teohhanhui teohhanhui deleted the revert-merge-master-into-2.2 branch May 2, 2018 17:17
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

9 participants