JoinedSubclassPersister pass identifier types on delete #7322

dennisenderink · 2018-07-24T14:48:58Z

Had trouble deleting entities that implemented the class inheritance strategy in combination with ramsey' uuid custom type. Turned out the identifier types weren't passed to the connection delete method in the JoinedSubclassPersister class.

It seems I wasn't the only one hence #6075 and #5988 so here is a fix.

This is my first PR into a public repository so hopefully I've done everything according the rules. If not, let me know!

Ocramius

Patch and approach are good - will need to improve some details, but overall the approach is correct 👍

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

tests/Doctrine/Tests/DbalTypes/CustomIdObjectType.php

dennisenderink · 2018-07-24T15:39:13Z

Updated patch as requested!

oleg-andreyev · 2018-09-10T15:06:13Z

Today I've caught same issue (v2.5.14)

dennisenderink · 2018-09-10T15:32:20Z

@Ocramius @guilhermeblanco what can I do to get this fix merged? (will update code with upstream soon)

Ocramius

LGTM 👍

Ocramius · 2018-09-11T08:07:54Z

Note to self: tests are to be ported to master too.

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

lcobucci · 2018-11-20T12:15:49Z

I've move this to v2.6.4 since we still have some investigation to be done

r3m4k3 · 2019-03-05T14:15:10Z

@Ocramius @Majkl578 Any update on this?

SenseException · 2019-03-05T22:53:39Z

@dennisenderink Anything that needs to be done from your side or is this ready for another review?

dennisenderink · 2019-03-06T05:40:44Z

@Ocramius @Majkl578 Any update on this?

@dennisenderink Anything that needs to be done from your side or is this ready for another review?

I just came home after a few months of traveling, didn't have the time to check @Majkl578 feedback before that. I will have some time later this week!

lukikrk · 2019-03-08T11:57:56Z

@dennisenderink @Ocramius @Majkl578
Is something already known?

kubapyla · 2019-03-12T11:33:33Z

@dennisenderink @Ocramius @Majkl578 @lukikrk
Are there any new updates on this?

dennisenderink · 2019-03-13T20:22:34Z

@dennisenderink @Ocramius @Majkl578 @lukikrk
Are there any new updates on this?

I updated the code last week but it doesn't seem to pass the tests because it is unable to install MySQL, which I think has nothing to do with the changes.

Will this be fixed in a separate PR?

If I'm not mistaken it is caused by this error in command sudo apt-get update -q in ./tests/travis/install-mysql-5.7.sh:

W: The repository 'http://repo.mysql.com/apt/ubuntu trusty InRelease' is not signed.
W: http://ppa.launchpad.net/couchdb/stable/ubuntu/dists/trusty/Release.gpg: Signature by key 15866BAFD9BCC4F3C1E0DFC7D69548E1C17EAB57 uses weak digest algorithm (SHA1)
W: GPG error: https://packagecloud.io/github/git-lfs/ubuntu trusty InRelease: The following signatures couldn't be verified because the public key is not available: NO_PUBKEY 6B05F25D762E3157
W: The repository 'https://packagecloud.io/github/git-lfs/ubuntu trusty InRelease' is not signed.
W: There is no public key available for the following key IDs:
6B05F25D762E3157

r3m4k3 · 2019-03-20T14:56:53Z

@Ocramius @Majkl578
What can we do with this issue?
As I understand @dennisenderink fixed the issue, but there's an error on Travis CI, could you please help us to sort this out?

dennisenderink · 2019-03-20T15:11:10Z

@Ocramius @Majkl578
What can we do with this issue?
As I understand @dennisenderink fixed the issue, but there's an error on Travis CI, could you please help us to sort this out?

That's correct!

SenseException · 2019-03-20T15:38:54Z

The builds using DB=mysql MYSQL_VERSION=5.7 still show the following error after I restarted the build:

WARNING: The following packages cannot be authenticated!
  mysql-common libmysqlclient20 mysql-community-client mysql-client
  mysql-community-server libmysqlclient-dev mysql-server
E: There were unauthenticated packages and -y was used without --allow-unauthenticated
The command "./tests/travis/install-mysql-$MYSQL_VERSION.sh" failed and exited with 100 during .

lukikrk · 2019-03-25T09:38:08Z

Could you recommend any workaround to solve this problem?

lukikrk · 2019-03-25T11:01:46Z

Detected the same situation during select with join operation.

…implemented in JoinedSubclassPersister delete method

lcobucci

I'll be applying these changes 👍

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

tests/Doctrine/Tests/ORM/Functional/ClassTableInheritanceCustomIdObjectTypeTest.php

…method return type

Which provides better isolation and makes things a bit more understandable.

Removing the unnecessary code duplication.

lcobucci

@dennisenderink thanks a lot for your contribution (and patience)!

dennisenderink · 2019-09-20T14:57:12Z

@dennisenderink thanks a lot for your contribution (and patience)!

No worries, glad I could help! Thanks for the changes, approval and merge! 👍

@mlocati

v2.6.4 [![Build Status](https://travis-ci.org/doctrine/orm.svg?branch=v2.6.4)](https://travis-ci.org/doctrine/orm) In this release we've fixes many bugs (including a performance regression) and made the v2.x series compatible with PHP 7.4. -------------------------------------------- - Total issues resolved: **11** - Total pull requests resolved: **32** - Total contributors: **30** Improvement ----------- - [7785: Fix "access array offset on value of type null" PHP 7.4 notices](doctrine#7785) thanks to @mlocati - [7142: Rename this repository to doctrine/orm](doctrine#7142) thanks to @greg0ire Bug ------------------ - [7821: Bug: doctrine#7820 paginator ignores dbal type conversions in identifiers](doctrine#7821) thanks to @Ocramius - [7778: Guard L2C regions against corrupted data](doctrine#7778) thanks to @umpirsky - [7767: PersistentCollection::matching() does not respect the collections native sorting](doctrine#7767) thanks to @stephanschuler - [7766: Respect collection orderBy meta when matching()](doctrine#7766) thanks to @stephanschuler - [7761: Do not modify UOW on PersistentCollection::clear() when owner has DEFFERED&doctrine#95;EXPLICIT change tracking policy](doctrine#7761) thanks to @paxal - [7750: Fix incorrect return of null values in L2C](doctrine#7750) thanks to @AlexSmerw - [7737: Fix MEMBER&doctrine#95;OF comparison when using criteria in query builder](doctrine#7737) thanks to @Smartel1 - [7735: Null in fields value in Cached Entity several times on day on high-load project.](doctrine#7735) thanks to @AlexSmerw - [7630: Fix doctrine#7629 - `scheduledForSynchronization` leaks memory when using `@ORM\ChangeTrackingPolicy("DEFERRED&doctrine#95;EXPLICIT")`](doctrine#7630) thanks to @yethee - [7528: Prevent `UnitOfWork` lookup for DBAL types specified in `Doctrine\ORM\Query#setParameter()`](doctrine#7528) thanks to @Ocramius - [7322: JoinedSubclassPersister pass identifier types on delete](doctrine#7322) thanks to @dennisenderink and @fred-jan - [7266: Call to a member function resolveAssociationEntries() on boolean {"detail":"&doctrine#91;object&doctrine#93; (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Call to a member function resolveAssociationEntries() on boolean at /www/vendor/doctrine/orm/lib/Doctrine/ORM/Cache/DefaultQueryCache.php:140)"}](doctrine#7266) thanks to @mingmingxianseng - [4632: DDC-3789: Paginator does not convert entity ids if they are value objects](doctrine#4632) thanks to @doctrinebot Documentation ------------- - [7818: Add note into docs about not using SimpleAnnotationReader](doctrine#7818) thanks to @SenseException - [7791: Fix preFlush event documentation stating incorrectly that flush can be called safely](doctrine#7791) thanks to @Steveb-p - [7753: Add ORM annotations in getting-started docs](doctrine#7753) thanks to @SenseException and @wajdijurry - [7744: Fixed a typo-error](doctrine#7744) thanks to @noobshow - [7732: &doctrine#91;Documentation&doctrine#93; Missing comma fix](doctrine#7732) thanks to @lchrusciel - [7729: Update DATE&doctrine#95;ADD and DATE&doctrine#95;SUB docs](doctrine#7729) thanks to @JoppeDC - [7672: Added cross-links to relevant documentation](doctrine#7672) thanks to @jschaedl - [7612: Update ordered-associations.rst](doctrine#7612) thanks to @spirlici - [7610: Change APC to OPcache in improving-performance.rst ](doctrine#7610) thanks to @smtchahal - [7596: Correct method names and broken link in docs](doctrine#7596) thanks to @mbessolov - [7577: Fix of single link to dbal docs in advanced-configuration.rst](doctrine#7577) thanks to @SenseException - [7572: Remove codeigniter Framework example](doctrine#7572) thanks to @SenseException - [7571: Fix typo in inheritance mappings docs](doctrine#7571) thanks to @batwolf - [7557: Change Stackoverflow tag to doctrine-orm](doctrine#7557) thanks to @malarzm - [7551: &doctrine#91;2.6&doctrine#93; Migrate repository name doctrine/doctrine2 -> doctrine/orm](doctrine#7551) thanks to @Majkl578 - [7530: Documentation error typo fix: s/Used-defined/User-Defined](doctrine#7530) thanks to @vladyslavstartsev - [7519: doctrine#7518 Fixed type mismatch between `EntityRepository#&doctrine#95;&doctrine#95;construct()` and its documented constructor arguments](doctrine#7519) thanks to @koftikes - [7518: `EntityRepository::&doctrine#95;&doctrine#95;construct()` expects `Doctrine\ORM\EntityManager` instead of actual required `EntityManagerInterface`](doctrine#7518) thanks to @koftikes - [7490: Fix broken link](doctrine#7490) thanks to @vladyslavstartsev - [7483: Fixed a minor syntax issue](doctrine#7483) thanks to @javiereguiluz CI ----------------- - [7794: Fix test compatibility with DBAL 2.10.x-dev](doctrine#7794) thanks to @lcobucci - [7731: Replace custom install script with add-on](doctrine#7731) thanks to @greg0ire - [7473: Incremental CS checks in 2.x branches](doctrine#7473) thanks to @Majkl578

Ocramius suggested changes Jul 24, 2018

View reviewed changes

Ocramius added the Bug label Jul 24, 2018

Ocramius added this to the 2.6.3 milestone Jul 24, 2018

Ocramius assigned dennisenderink Jul 24, 2018

Ocramius previously approved these changes Sep 11, 2018

View reviewed changes

Ocramius added the Requires merge/review label Sep 11, 2018

Majkl578 suggested changes Sep 23, 2018

View reviewed changes

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php Show resolved Hide resolved

lcobucci modified the milestones: 2.6.3, 2.6.4 Nov 20, 2018

dennisenderink dismissed Ocramius’s stale review via dcb48e1 March 6, 2019 07:15

Separated class identifiers types retrieval in a separate method and …

cfe73cd

…implemented in JoinedSubclassPersister delete method

lcobucci force-pushed the fix/joinedsubclasspersister-pass-identifier-types-on-delete branch from dcb48e1 to 9f5f594 Compare September 20, 2019 13:40

lcobucci suggested changes Sep 20, 2019

View reviewed changes

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php Outdated Show resolved Hide resolved

tests/Doctrine/Tests/ORM/Functional/ClassTableInheritanceCustomIdObjectTypeTest.php Outdated Show resolved Hide resolved

lcobucci removed the Requires merge/review label Sep 20, 2019

lcobucci assigned lcobucci and unassigned dennisenderink Sep 20, 2019

lcobucci mentioned this pull request Sep 20, 2019

JoinedSubclassPersister::delete does not pass $types argument to Connection::delete #5988

Closed

lcobucci added the Pick to master label Sep 20, 2019

dennisenderink and others added 7 commits September 20, 2019 16:17

Pass ClassMetadata object instead of string in parent classes loop

d7a537c

Added unit tests

37d1d57

Created new DBAL type and updated/reverted tests; also clarified new …

390d081

…method return type

Patched spacing

59f1679

Added correct return types to docblock

53775fe

Centralise functional test classes

435d624

Which provides better isolation and makes things a bit more understandable.

Make use of the PersisterHelper to get field type

ef783f7

Removing the unnecessary code duplication.

lcobucci force-pushed the fix/joinedsubclasspersister-pass-identifier-types-on-delete branch from 8758908 to ef783f7 Compare September 20, 2019 14:17

lcobucci approved these changes Sep 20, 2019

View reviewed changes

lcobucci merged commit b52ef5a into doctrine:2.6 Sep 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JoinedSubclassPersister pass identifier types on delete #7322

JoinedSubclassPersister pass identifier types on delete #7322

dennisenderink commented Jul 24, 2018

Ocramius left a comment

dennisenderink commented Jul 24, 2018

oleg-andreyev commented Sep 10, 2018

dennisenderink commented Sep 10, 2018

Ocramius left a comment

Ocramius commented Sep 11, 2018

lcobucci commented Nov 20, 2018

r3m4k3 commented Mar 5, 2019

SenseException commented Mar 5, 2019

dennisenderink commented Mar 6, 2019

lukikrk commented Mar 8, 2019

kubapyla commented Mar 12, 2019

dennisenderink commented Mar 13, 2019 •

edited

Loading

r3m4k3 commented Mar 20, 2019

dennisenderink commented Mar 20, 2019

SenseException commented Mar 20, 2019

lukikrk commented Mar 25, 2019

lukikrk commented Mar 25, 2019

lcobucci left a comment

lcobucci left a comment

dennisenderink commented Sep 20, 2019

JoinedSubclassPersister pass identifier types on delete #7322

JoinedSubclassPersister pass identifier types on delete #7322

Conversation

dennisenderink commented Jul 24, 2018

Ocramius left a comment

Choose a reason for hiding this comment

dennisenderink commented Jul 24, 2018

oleg-andreyev commented Sep 10, 2018

dennisenderink commented Sep 10, 2018

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius commented Sep 11, 2018

lcobucci commented Nov 20, 2018

r3m4k3 commented Mar 5, 2019

SenseException commented Mar 5, 2019

dennisenderink commented Mar 6, 2019

lukikrk commented Mar 8, 2019

kubapyla commented Mar 12, 2019

dennisenderink commented Mar 13, 2019 • edited Loading

r3m4k3 commented Mar 20, 2019

dennisenderink commented Mar 20, 2019

SenseException commented Mar 20, 2019

lukikrk commented Mar 25, 2019

lukikrk commented Mar 25, 2019

lcobucci left a comment

Choose a reason for hiding this comment

lcobucci left a comment

Choose a reason for hiding this comment

dennisenderink commented Sep 20, 2019

dennisenderink commented Mar 13, 2019 •

edited

Loading