Skip to content

[Paginator] Fix regression when using Custom ID type#7903

Closed
ostrolucky wants to merge 3 commits intodoctrine:2.6from
ostrolucky:7890-paginator-objecti
Closed

[Paginator] Fix regression when using Custom ID type#7903
ostrolucky wants to merge 3 commits intodoctrine:2.6from
ostrolucky:7890-paginator-objecti

Conversation

@ostrolucky
Copy link
Copy Markdown
Member

@ostrolucky ostrolucky commented Nov 16, 2019 •
edited
Loading

Continuation of #7890. Created new PR to clean the mess with huge diff, though I still left attributions via co-authors.

I have reused previous code from @Ocramius for getting id type, however if preferred, I can replace it with this smaller piece of code:

$AST           = $subQuery->getAST();
$entityManager = $subQuery->getEntityManager();
$connection    = $this->query->getEntityManager()->getConnection();
$classMetadata = $entityManager->getClassMetadata(
    $AST->fromClause->identificationVariableDeclarations[0]->rangeVariableDeclaration->abstractSchemaName
);

$identifierType = PersisterHelper::getTypeOfField(
    $classMetadata->getSingleIdentifierFieldName(),
    $classMetadata,
    $entityManager
)[0];

$ids =  array_map(static function(array $row) use ($identifierType, $connection) {
    return current($row);
    return $connection->convertToPHPValue(current($row), $identifierType);
}, $foundIdRows);

Please check if this approach is right one, as it's weird that we are now doing PHP<>DB conversions in both Paginator and WhereInWalker

@ostrolucky ostrolucky added the Bug label Nov 16, 2019
@ostrolucky ostrolucky added this to the 2.6.5 milestone Nov 16, 2019
@ostrolucky ostrolucky self-assigned this Nov 16, 2019
@ostrolucky ostrolucky mentioned this pull request Nov 16, 2019
Closed
@lcobucci
Copy link
Copy Markdown
Member

lcobucci commented Nov 16, 2019

@ostrolucky this is basically what @Ocramius had in 39d2113 and then moved to the walker in c67a515. Which basically invalidades the changes in WhereInWalker...

I'd still prefer to keep things contained in the WhereInWalker as much as possible, have you found the root cause of why the tests fail when using custom query walkers?

@ostrolucky
Copy link
Copy Markdown
Member Author

ostrolucky commented Nov 16, 2019 •
edited
Loading

It's not same. In 39d2113 was convertToDatabaseValue call happening in Paginator. It was moved in c67a515 into WhereInWalker. What I've done here is to call convertToPHPValue in Paginator. This is a reverse call. Without that, WhereInWalker will have access to ids already in database values only. That said, I don't know how much this bug has to do with custom walkers, but it's the only failing testcase we received.

@lcobucci
Copy link
Copy Markdown
Member

lcobucci commented Nov 16, 2019

@ostrolucky you're right. However, I think this should have been solved by the hydrator already. I'm debugging it now.

@ostrolucky
Copy link
Copy Markdown
Member Author

ostrolucky commented Nov 16, 2019

Thanks. Initially I wanted to just call getResult() in Paginator, but that didn't turn out to be easy.

@lcobucci
Copy link
Copy Markdown
Member

lcobucci commented Nov 16, 2019 •
edited
Loading

@ostrolucky it's related to this wonderful piece of code:

// WARNING: BC break! We know this is the desired behavior to type convert values, but this
// erroneous behavior exists since 2.0 and we're forced to keep compatibility.

The LimitSubqueryWalker creates a field which isn't mapped, causing this mess.

@lcobucci
Copy link
Copy Markdown
Member

lcobucci commented Nov 16, 2019

Yeah, I managed to solve this only by addressing the issue in AbstractHydrator. Didn't do it pretty because I wanted to validate my thoughts...

I believe we should ensure that things are properly hydrated in the hydrator instead of pulling this responsibility to the paginator. Do you have any concerns @Ocramius @guilhermeblanco?

@ostrolucky
Copy link
Copy Markdown
Member Author

ostrolucky commented Nov 16, 2019

I like that idea

@akorz
Copy link
Copy Markdown
Contributor

akorz commented Nov 16, 2019

@ostrolucky Thank you for help :)

ostrolucky and others added 3 commits November 16, 2019 12:19
@ostrolucky @akorz
[Paginator] Add test case for regression with custom id
036f34e 
Co-authored-by: Alexei Korolev <alexei.korolev@gmail.com>
@ostrolucky @Ocramius
Backport helpers for getting ID type from Query
afcb9bb 
Backported from 39d2113

Co-authored-by: Marco Pivetta <ocramius@gmail.com>
@ostrolucky
[Paginator] Fix regression when using Custom ID type
e86e271 
Regression from fixing #7837
@ostrolucky ostrolucky changed the title [Paginator] Fix regression when using Custom ID type and custom query walkers [Paginator] Fix regression when using Custom ID type Nov 16, 2019
@ostrolucky
Copy link
Copy Markdown
Member Author

ostrolucky commented Nov 16, 2019

I have simplified the test case more. It doesn't have anything to do with custom walkers indeed. @lcobucci can we see your patch?

@lcobucci lcobucci mentioned this pull request Nov 17, 2019
Merged
@lcobucci
Copy link
Copy Markdown
Member

lcobucci commented Nov 18, 2019

Superseded by #7905

@lcobucci lcobucci closed this Nov 18, 2019
@lcobucci lcobucci removed this from the 2.6.5 milestone Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ocramius Ocramius Awaiting requested review from Ocramius

@lcobucci lcobucci Awaiting requested review from lcobucci

Assignees

@ostrolucky ostrolucky

Labels

Bug Regression

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ostrolucky @lcobucci @akorz