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

Support using embeddables in criterias with ArrayCollection #216

Merged
merged 3 commits into from
Nov 4, 2021

Conversation

garak
Copy link
Contributor

@garak garak commented Nov 13, 2019

This is a re-proposal of PR #27 by @mnapoli

@mnapoli
Copy link
Contributor

mnapoli commented Nov 13, 2019

I wanted to stop being pinged on the last PR and now I just got 4 new pings this morning…

@garak
Copy link
Contributor Author

garak commented Nov 13, 2019

@mnapoli I'm sorry you feel bothered. I guess github is already providing tools to avoid notifications.

@garak
Copy link
Contributor Author

garak commented Jan 13, 2020

I know it's not nice to bump, but two months passed and I'm still waiting for a review.

@greg0ire
Copy link
Member

greg0ire commented Jan 13, 2020

You are targeting master, is it because there is a BC-break in there? Are dots already supported? What is the current behavior regarding them?

@garak
Copy link
Contributor Author

garak commented Jan 13, 2020

You are targeting master, is it because there is a BC-break in there? Are dots already supported? What is the current behavior regarding them?

I follow "everything to master first" rule.
Dots are currently not supported and this PR aims to add such support

@greg0ire
Copy link
Member

greg0ire commented Jan 13, 2020

I follow "everything to master first" rule.

Just checking, we probably need to change the default branch here.
Since your PR seems BC, please target 1.6.x

Dots are currently not supported and this PR aims to add such support

So there is a crash?

@garak garak changed the base branch from master to 1.6.x January 14, 2020 07:31
@garak
Copy link
Contributor Author

garak commented Jan 14, 2020

I follow "everything to master first" rule.

Just checking, we probably need to change the default branch here.
Since your PR seems BC, please target 1.6.x

Tried to, but it's a mess. Tried to rebase, with much success. Do you think it would be better to re-open a new PR with relevant changes?

Dots are currently not supported and this PR aims to add such support

So there is a crash?

I guess it would just don't work.

@greg0ire
Copy link
Member

I'm going to rebase it interactively for you.

@greg0ire
Copy link
Member

Here are roughly the steps I'm going to follow FYI:

  1. Fetch all new commits: git fetch --all. ⬇️
  2. Rebase on what you fetched interactively: ✂️
    1. git rebase -i origin/1.6.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin repository.
    2. A window will show up with many lines, remove every line but the last one, which should correspond to your commit.
    3. If you run into a conflict: 💥
      1. fix it with git mergetool. 😷
      2. continue on your merry way with git rebase --continue. ⏩
  3. Force push to overwrite all this : git push --force. ⬆️

greg0ire
greg0ire previously approved these changes Jan 14, 2020
@greg0ire
Copy link
Member

Are there any docs that need to be updated?

@garak
Copy link
Contributor Author

garak commented Jan 14, 2020

Are there any docs that need to be updated?

I took a look to current docs but I couldn't find a spot to possibly add something relevant.
This is just a fix for something that is expect to work, but did not

@greg0ire
Copy link
Member

Thanks for checking!

@greg0ire
Copy link
Member

greg0ire commented Feb 5, 2020

This is a feature, so I'm going to retarget to 1.7.x

@greg0ire greg0ire dismissed their stale review February 5, 2020 18:06

The base branch was changed.

@greg0ire greg0ire changed the base branch from 1.6.x to 1.7.x February 5, 2020 18:06
greg0ire
greg0ire previously approved these changes Feb 5, 2020
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

That comment was automated, both branches correspond to the same commit so my approval stays.

@garak
Copy link
Contributor Author

garak commented Oct 30, 2021

I just solved conflicts

@greg0ire greg0ire added this to the 1.7.0 milestone Nov 4, 2021
@greg0ire greg0ire merged commit 0163afb into doctrine:1.7.x Nov 4, 2021
@greg0ire
Copy link
Member

greg0ire commented Nov 4, 2021

Thanks @garak !

@garak garak deleted the arraycollection-embeddable branch November 4, 2021 07:30
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

4 participants