Fix #5923 - disambiguate identifier hashing

[Ocramius]

Fixes #5923

Requires #5924 to be merged first.

[beberlei]

if (!isset($identifier[1])) {
     return (string)$identifier[0];
}

[Ocramius]

Sadly, $identifier is not guaranteed to be 0-indexed. More keys may exist after that, with undefined values

[lcobucci]

@Ocramius @guilhermeblanco what should we do with this one?

[Ocramius]

@lcobucci I'd say that we need to add benchmarks for these code paths

[lcobucci]

@Ocramius alright should we move it to v2.7 or maybe v2.6.1 then?

[Ocramius]

@lcobucci 3.x

[lcobucci]

@Ocramius any reason for moving it back to 2.6.x?

[Ocramius]

I want to give this another shot before abandoning all hope: it's a bad one and it should die

…

On 26 Jun 2017 11:13 PM, "Luís Cobucci" ***@***.***> wrote: @Ocramius <https://github.com/ocramius> any reason for moving it back to 2.6.x? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5926 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakDFthabem878_aMG9h6rhiO7aWQdks5sIB8XgaJpZM4JHdNs> .

[lcobucci]

Benchmark results (with XDebug and without opcache - using #6575)

Test	Time (μs) - before	Time (μs) - after	Diff (μs)
UnitOfWorkComputeChangesBench#benchChangeSetComputation	12,055	12,338	+283
MixedQueryFetchJoinArrayHydrationPerformanceBench#benchHydration	437,631	441,650	+4,019
MixedQueryFetchJoinFullObjectHydrationPerformanceBench#benchHydration	455,700	494,812	+39,112
MixedQueryFetchJoinPartialObjectHydrationPerformanceBench#benchHydration	242,108	262,340	+20,232
SimpleHydrationBench#benchHydration	1,338,824	1,395,454	+56,630
SimpleInsertPerformanceBench#benchHydration	3,647,768	3,666,269	+18,501
SimpleQueryArrayHydrationPerformanceBench#benchHydration	243,605	245,064	+1,459
SimpleQueryFullObjectHydrationPerformanceBench#benchHydration	1,608,842	1,742,565	+133,723
SimpleQueryPartialObjectHydrationPerformanceBench#benchHydration	489,523	578,972	+89,449
SimpleQueryScalarHydrationPerformanceBench#benchHydration	183,123	211,087	+27,964
SingleTableInheritanceHydrationPerformanceBench#benchHydrateFixContracts	5,822	6,949	+1,127
SingleTableInheritanceHydrationPerformanceBench#benchHydrateFlexContracts	9,851	10,972	+1,121
SingleTableInheritanceHydrationPerformanceBench#benchHydrateUltraContracts	6,610	7,475	+865
SingleTableInheritanceHydrationPerformanceBench#benchHydrateAllContracts	12,951	14,048	+1,097
SingleTableInheritanceInsertPerformanceBench#benchInsertFixContracts	14,536	15,768	+1,232
SingleTableInheritanceInsertPerformanceBench#benchInsertFlexContracts	15,428	18,049	+2,621
SingleTableInheritanceInsertPerformanceBench#benchInsertUltraContracts	16,413	17,102	+689
SingleTableInheritanceInsertPerformanceBench#benchInsertAllContracts	33,686	38,128	+4,442
ProxyInitializationTimeBench#benchCmsUserInitialization	645,743	680,637	+34,894
ProxyInitializationTimeBench#benchCmsEmployeeInitialization	142,335	152,550	+10,215
ProxyInitializationTimeBench#benchInitializationOfAlreadyInitializedCmsUsers	9,178	9,249	+71
ProxyInitializationTimeBench#benchInitializationOfAlreadyInitializedCmsEmployees	9,065	8,666	-399
ProxyInstantiationTimeBench#benchCmsUserInstantiation	993,616	1,057,435	+63,819
ProxyInstantiationTimeBench#benchCmsEmployeeInstantiation	448,051	532,088	+84,037

Summary

Metric	Before	After
best	5,822.000	6,949.000
mean mode	459,269.333 459,269.333	484,152.792 484,152.792
worst	5,822.000	6,949.000
⅀T	11,022,464.000μs	11,619,667.000μs

[Ocramius]

That is a lot. Assuming that identifiers don't change, except for generated identifiers, the hashing should probably be cached...

[lcobucci]

@Ocramius I've rebased this branch to have phpbench, so the diff is:
master: ⅀T: 7,077,983.000μs
this PR: ⅀T: 5,950,296.000μs

But Travis' containers might not be 100% reliable regarding performance, idk.

[guilhermeblanco]

This PR is wrong. It introduces a behavior worse than we have today.
Let me quickly try to explain: If you have an entity that contains an association as identifier and the target entity implements __toString(), everything breaks.

I'm at the point where I have to properly address this issue for once and for all.
The solution is to NOT store flattened identifiers in $uow->entityIdentifiers. It's also not to hash using any kind of implode(), array_map() or any transformation.
It should purely get the identifier values (in master, $metadata->getIdentifierValues(); in develop, $persister->getIdentifier()) and call serialize(). You can see why this is required since the UnitOfWork::isIdentifierEquals() method is currently broken, since flattened identifiers and entityIdentifiers can be different today.
Using serialize() will allow developers to also control the hash if they want (by implementing Serializable).

As soon as you make these type of changes, all EntityPersisters break. The reason is because association identifiers can only go 1 level deep (associated id whose target entity also has associated id are not supported). This would require to completely rewrite both prepareInsertData and prepareUpdateData. I've done 50% of the work already in develop, but I consider it worthless to try in master.

IMHO, we should just close this PR.

[lcobucci]

@Ocramius let's close this then?

[Ocramius]

Closing is fine by me, but we still need to solve the ID => object problem, and hashing is a semi-decent solution. My last attempt used the SplObjectMap (or whatever it was called), and performance was dreadful. Unless we enforce usage of the php-dd extension, I don't see this happening 😥

…

On 6 Aug 2017 12:56 PM, "Luís Cobucci" ***@***.***> wrote: @Ocramius <https://github.com/ocramius> let's close this then? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5926 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakPCQwGeHTJUkq2qrE4woHrFKPjguks5sVZvSgaJpZM4JHdNs> .

[Ocramius]

Delaying to 3.0 - while this is a critical bug, fixing it now is more trouble, as we'd only introduce more dirty hacks.

[derrabus]

I'm closing this PR because it hasn't seen progress for several years. Please open a new one if you want to pick up the work one day.