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 matching lazy collections with enums criteria #11516

Open
wants to merge 6 commits into
base: 3.2.x
Choose a base branch
from

Conversation

kira0269
Copy link

@kira0269 kira0269 commented Jun 21, 2024

Fix issue #11481

I identified some duplicated code between the BasicEntityPersister and the ManyToManyPersister, like the method expandCriteriaParameters.
Because I don't want to create side effects, I just factorized two methods : getValues and getIndividualValue into a trait.

Thanks for your review !

@kira0269 kira0269 force-pushed the fix_issue_11481 branch 5 times, most recently from d38e933 to f9dbfdf Compare June 24, 2024 07:24
@derrabus
Copy link
Member

Please find a meaningful title for your PR. We use the titles in our automatically composed change log.

src/Persisters/Collection/ManyToManyPersister.php Outdated Show resolved Hide resolved
src/Persisters/Traits/ResolveValuesHelper.php Show resolved Hide resolved
src/Persisters/Traits/ResolveValuesHelper.php Outdated Show resolved Hide resolved
tests/Tests/ORM/Functional/EnumTest.php Outdated Show resolved Hide resolved
tests/Tests/ORM/Functional/EnumTest.php Outdated Show resolved Hide resolved
@kira0269 kira0269 changed the title Fix issue #11481 Fix matching lazy collection with enums criteria Jun 24, 2024
@kira0269 kira0269 changed the title Fix matching lazy collection with enums criteria Fix matching lazy collections with enums criteria Jun 24, 2024
@kira0269 kira0269 requested a review from derrabus June 24, 2024 08:54
@kira0269
Copy link
Author

Hello @derrabus
Could you review this PR please ?
Also, the change you requested was applied on the first commit, it's still active despite the fact the file has been updated.
Thank you 😄

@kira0269
Copy link
Author

kira0269 commented Aug 6, 2024

Up @derrabus . This PR is still stuck because of your ghost request change 😢

@kira0269
Copy link
Author

kira0269 commented Aug 7, 2024

Hi,

I've got these issues from Psalm:

Error: src/Persisters/Entity/BasicEntityPersister.php:0:0: UnusedBaselineEntry: Baseline for issue "LessSpecificReturnStatement" has 3 extra entries. (see https://psalm.dev/316)
Error: src/Persisters/Entity/BasicEntityPersister.php:0:0: UnusedBaselineEntry: Baseline for issue "MoreSpecificReturnType" has 3 extra entries. (see https://psalm.dev/316)

And I don't understand what it means... I don't know how to fix those "issues".

Thanks for your help.

@SenseException
Copy link
Member

Your changes seem to have removed code with psalm issues that is still ignored in the psalm baseline and don't need to be anymore. If you find the affected lines in the XML for BasicEntityPersister class and remove them, then the UnusedBaselineEntrys should be gone. Some of the ignored BasicEntityPersister issues could still be valid.

@kira0269
Copy link
Author

Hi @derrabus

I don't know why, but the merging is blocked because of a change you requested on one of the commits of this branch.
I applied the change, but github does not detect it. So could you remove your requested change please ?

Thanks a lot.

Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, good catch and work on this!

A few suggestions, then its ready to merge from my POV.

$paramTypes[] = PersisterHelper::getTypeOfField($name, $targetClass, $this->em)[0];
}
}

$params = array_merge($params, ...$paramsValues);
unset($paramsValues);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this unset, its not needed.

$this->_em->flush();
$libraryId = $library->id;

unset($library, $redBook, $blueBook);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this unset, its not needed.

$this->_em->flush();
$libraryId = $library->id;

unset($library, $redBook, $blueBook);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this unset, its not needed.

$thrillerCategoryId = $thrillerCategory->id;

$this->_em->clear();
unset($thrillerCategory, $fantasyCategory, $blueBook, $redBook);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this unset, its not needed.

$thrillerCategoryId = $thrillerCategory->id;

$this->_em->clear();
unset($thrillerCategory, $fantasyCategory, $blueBook, $redBook);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this unset, its not needed.


public function getBooksWithColor(BookColor $bookColor): Collection
{
$criteria = Criteria::create()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if you move this code into the test, because reading the test code now its not easy to grasp how test name matches to code. Same for the other Criteria code in Library.

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.

4 participants