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

Support Enum IDs and search by Enum fields #9629

Merged
merged 11 commits into from
Apr 9, 2022
Merged

Support Enum IDs and search by Enum fields #9629

merged 11 commits into from
Apr 9, 2022

Conversation

michnovka
Copy link
Contributor

This is a 2.12.x branch PR from #9628 and #9624

It implements:

  1. using Enum fields as ID columns
  2. using both old Repository->find(SomeEnum::XXX->value) and Repository->find(SomeEnum::XXX) syntax
  3. UnitOfWork::tryGetById() now supports Enum IDs as well as ID flattening when foreign identifiers are used (before this had to be handled by the caller, but it was on some and was not on other places)

@beberlei
Copy link
Member

beberlei commented Apr 5, 2022

This is becoming a larger rabbit hole than anticipated to support enum ids. I am unsure about proceeding with thiS as I currently have little tile to spare for review due to private life.

@michnovka
Copy link
Contributor Author

@beberlei lol, tell me about it. its 2am here and what started as a simple fix turned out to 6 hour nightmare. I see some issues still, for some reason ClassMetadataInfo::validateAndCompleteFieldMapping() does not seem to be called in some instances, therefore $containsEnumIdentifier is not set to true. I will resume work on this tomorrow, see if I can find the cause and fix it.

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.

Note to fuether review: UoW additions potentially break second level cache

lib/Doctrine/ORM/UnitOfWork.php Show resolved Hide resolved
@michnovka
Copy link
Contributor Author

Note to fuether review: UoW additions potentially break second level cache

Aha! Thanks, I have pushed new commit which fixes the issue thanks to your guidance

@michnovka michnovka marked this pull request as draft April 5, 2022 07:33
@michnovka michnovka marked this pull request as ready for review April 5, 2022 11:35
@michnovka
Copy link
Contributor Author

@beberlei I have removed some of the more controversial changes which were causing issues with tryGetById(). That method is now unchanged. The latest commit passes all tests. There is no BC break. Please run workflows and review. Thanks!

@michnovka michnovka requested a review from derrabus April 5, 2022 15:51
@michnovka
Copy link
Contributor Author

All tests passing now, can we have a merge please?

@derrabus derrabus added this to the 2.12.0 milestone Apr 9, 2022
@derrabus derrabus merged commit a3d82f8 into doctrine:2.12.x Apr 9, 2022
@derrabus
Copy link
Member

derrabus commented Apr 9, 2022

Thank you @michnovka!

derrabus added a commit to derrabus/orm that referenced this pull request Apr 11, 2022
* 2.12.x:
  Leverage generic persistence event classes (doctrine#9633)
  Fix static analysis for Persistence 2.5 (doctrine#9648)
  Improve exception message (doctrine#9646)
  Deprecate console helper (doctrine#9641)
  Use charset/collation from column or table default when creating relations (doctrine#9636)
  Support Enum IDs and search by Enum fields (doctrine#9629)
  Fix composer install in contributing readme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants