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

Safeguard for identity map seems to break merge #10820

Closed
alexander-schranz opened this issue Jul 6, 2023 · 3 comments
Closed

Safeguard for identity map seems to break merge #10820

alexander-schranz opened this issue Jul 6, 2023 · 3 comments

Comments

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Jul 6, 2023

I'm currently running into the following issues since: #10785

Bildschirmfoto 2023-07-06 um 16 18 50

BC Break Report

Q A
BC Break yes
Version ^2@dev (current released version works)

Summary

For me it looks this currently breaks the existing EntityManager::merge method. I know merge is deprecated but it is used in this case as the object get not retriefied from the database instead unserialized and then crashes when it tries to merge into the entityManager. https://github.com/sulu/sulu/blob/f977ffc0f480cb1d4ddee9cde0cbb8a881011d91/src/Sulu/Bundle/AudienceTargetingBundle/Entity/TargetGroupRepository.php#L33

Should not merged entities not show this identiy map warning as they got merged and only persisted?

Previous behavior

Don't did crash on current implemented method.

Current behavior

Crashes with a new method.

How to reproduce

git clone [email protected]:sulu/sulu.git
composer update doctrine/orm:"@dev" --prefer-source

# start database
# export DATABASE_URL=...

bin/runtests -i -t AudienceTargetingBundle -C --flags="--filter=testPut"
@mpdude
Copy link
Contributor

mpdude commented Aug 2, 2023

Can you provide an isolated test case for ORM?

To avoid regressions, we need to have that test in our own suite, we cannot run Sulu's tests in our CI.

@mpdude
Copy link
Contributor

mpdude commented Aug 2, 2023

The object you’re trying to merge (which has been serialized previously, if I get you right), is that a proxy instance?

@alexander-schranz
Copy link
Contributor Author

I tested 2.17.3 and 2.17.4 today. It seems like the issue does not longer exist also I tested #10867 which seems also be fixed there I know it was fixed in 2.16.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants