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.

Sign up for GitHub

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

Jump to bottom

Fix ID of custom dbal types #267

Merged
merged 1 commit into from
Jan 11, 2020
Merged

Fix ID of custom dbal types #267

merged 1 commit into from
Jan 11, 2020

Conversation

JustBlackBird
Copy link
Contributor

@JustBlackBird JustBlackBird commented Aug 11, 2017 •
edited
Loading

This PR is a fix for #254

Not sure if I didn't break anything but the tests are passing 😄

@JustBlackBird
Copy link
Contributor Author

Rebased

Majkl578
Majkl578 reviewed Sep 7, 2017
View reviewed changes
tests/bootstrap.php Outdated Show resolved Hide resolved
@JustBlackBird
Copy link
Contributor Author

@Majkl578 I've moved type initialization to setUpBeforeClass. CI check is failed because it's unable to setup mongodb for php7.2 not because of the PR.

@JustBlackBird
Copy link
Contributor Author

Is there anything else I should fix to get the PR merged?

@kisprof
Copy link

kisprof commented Jun 27, 2018

Hi there!

Whats the status of this PR? I just run into the same issue as in #254 and i was glad to see there is already a PR for it. Can i help in any way to speed up the merging process?

@JustBlackBird
Copy link
Contributor Author

@Majkl578 ping

@JustBlackBird
Copy link
Contributor Author

@Ocramius @Majkl578
Is there anything I should fix to get the PR finally merged?

@alcaeus alcaeus requested a review from Majkl578 November 28, 2018 08:52
@JustBlackBird
Copy link
Contributor Author

I've rebased changes to fix merge conflict.

@JustBlackBird

This comment has been minimized.

Sign in to view
@alcaeus

This comment has been minimized.

Sign in to view
@greg0ire

This comment has been minimized.

Sign in to view
@JustBlackBird

This comment has been minimized.

Sign in to view
@greg0ire

This comment has been minimized.

Sign in to view
@greg0ire greg0ire closed this Jan 4, 2020
@greg0ire greg0ire reopened this Jan 4, 2020
@greg0ire
Copy link
Member

greg0ire commented Jan 4, 2020 •
edited
Loading

#333 is not merged up in master yet, hence why the build is not fixed. Are you sure you should target master though? Is there a BC-break in your PR?

EDIT: I don't think so, let me retarget this PR for you .

@greg0ire greg0ire changed the base branch from master to 1.4.x January 4, 2020 15:41
@greg0ire greg0ire closed this Jan 4, 2020
@greg0ire greg0ire reopened this Jan 4, 2020
@greg0ire

This comment has been minimized.

Sign in to view
@JustBlackBird
Copy link
Contributor Author

JustBlackBird commented Jan 7, 2020 •
edited
Loading

@greg0ire Thank you for your help with codding style

@greg0ire greg0ire requested review from a team and removed request for Majkl578 January 7, 2020 21:41
alcaeus
alcaeus reviewed Jan 10, 2020
View reviewed changes
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

👍 LGTM, but I'll defer to @greg0ire for a final review.

@alcaeus alcaeus requested review from greg0ire and removed request for a team January 10, 2020 14:13
@greg0ire
Copy link
Member

I amended your commit with a message that sums up what I believe is a correct explanation for your PR. Please tell me if I got it wrong.

greg0ire
greg0ire approved these changes Jan 10, 2020
View reviewed changes
Copy link
Member

@greg0ire greg0ire left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I reverted your changes in src locally, and the tests failed, so that's good.
I also tried removing the implementation of Serializable from Uuid, it didn't crash, so that great too. Will merge if you are OK with the new commit message.

@JustBlackBird
Copy link
Contributor Author

JustBlackBird commented Jan 11, 2020 •
edited
Loading

I amended your commit with a message that sums up what I believe is a correct explanation for your PR. Please tell me if I got it wrong.

@greg0ire The message is fine, thank you!

@greg0ire greg0ire merged commit 93d8e69 into doctrine:1.4.x Jan 11, 2020
@greg0ire
Copy link
Member

Great, thanks @JustBlackBird !

@JustBlackBird
Copy link
Contributor Author

@greg0ire thank you for getting the PR finally merged! 🎉

@greg0ire greg0ire added this to the 1.4.1 milestone Jan 13, 2020
@doctrinebot doctrinebot mentioned this pull request Jan 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alcaeus alcaeus alcaeus left review comments

@Majkl578 Majkl578 Majkl578 left review comments

@greg0ire greg0ire greg0ire approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4.1
Development

Successfully merging this pull request may close these issues.
5 participants
@JustBlackBird @kisprof @alcaeus @greg0ire @Majkl578