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

Fix regression in 2.7.1 when mysqli is used with discriminator column that is not a string #8055

Merged
merged 3 commits into from Mar 16, 2020

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Mar 4, 2020

In 2.7.1, #7974 was included as the fix for #7505. Basically, the change made was that when hydrating entities with inheritance, only the fields/data that belong to the particular inheritance type must be passed to UoW::createEntity().

To do so, the SimpleObjectHydrator will compare the discriminator column value against the list of known types/mappings.

// If we have inheritance in resultset, make sure the field belongs to the correct class
if (isset($cacheKeyInfo['discriminatorValues']) && ! in_array($discrColumnValue, $cacheKeyInfo['discriminatorValues'], true)) {
continue;
}

While the descriminator types in $cacheKeyInfo['discriminatorValues'] are always strings, the actual $discrColumnValue may be an int, at least when the discriminator column is defined as int and the mysqli driver is used.

The problem seems not to occur with pdo_mysql: In that case, the values returned from the result set/query are (always?) strings.

My suggested fix is to (string) cast the discriminator value, as it already happens in the AbstractHydrator:

// If there are field name collisions in the child class, then we need
// to only hydrate if we are looking at the correct discriminator value
if (isset($cacheKeyInfo['discriminatorColumn'], $data[$cacheKeyInfo['discriminatorColumn']])
&& ! in_array((string) $data[$cacheKeyInfo['discriminatorColumn']], $cacheKeyInfo['discriminatorValues'], true)
) {
break;
}

Update:

It seems Travis is running MySQL tests with pdo_mysql only, so the test case does not even show the issue. I am also sure that the issue would have surfaced with the existing tests.

I still believe the fix is correct, given the similarity to the code in AbstractHydrator.

Anyway, could someone advise if this pdo_mysql/mysqli discrepancy is something worth looking into?

@mpdude
Copy link
Contributor Author

mpdude commented Mar 5, 2020

What about running tests for mysqli?

I understand that the test matrix is already very complex and resource intensive.

But unless we run mysqli, the regression test is useless and we’re not going to find such bugs.

@beberlei beberlei added this to the 2.7.2 milestone Mar 15, 2020
@beberlei beberlei merged commit 58b8130 into doctrine:2.7 Mar 16, 2020
@mpdude mpdude deleted the non-string-discriminator branch January 7, 2021 07:37
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

Successfully merging this pull request may close these issues.

None yet

2 participants