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 enum IDs in association mappings #10274

Merged
merged 1 commit into from
Dec 8, 2022

Conversation

michnovka
Copy link
Contributor

Attempt to fix #10132

@michnovka
Copy link
Contributor Author

michnovka commented Dec 6, 2022

@greg0ire please review, this fixes a very specific case where we are using complex associations to entities with one or more ID columns, one of which is an enum.

My original thought was to use identifierFlattener like this:

$associatedIdFlattened = $this->identifierFlattener->flattenIdentifier($targetClass, $associatedId);
$relatedIdHash         = implode(' ', $associatedIdFlattened);

here -

$relatedIdHash = implode(' ', $associatedId);

But when running tests, this created some issues and BC breaks. I this this fix is sufficient. Other cases (like a single enum ID not part of association) already use checks like this

@michnovka michnovka changed the title Fix complex enum IDs Fix enum IDs in association mappings Dec 6, 2022
@greg0ire
Copy link
Member

greg0ire commented Dec 7, 2022

Please squash your commits together and use a commit message as good as this one: 9d5ab4c
You can read the contributing guide to get guidelines rather than an example.

@greg0ire
Copy link
Member

greg0ire commented Dec 7, 2022

a very specific case where we are using complex associations to entities with one or more ID columns, one of which is an enum

In your test, all 3 entities have the enum appear in the primary key, is that required to reproduce the issue?

@michnovka michnovka force-pushed the 2.13.x-complex-enum-ids branch 2 times, most recently from cf726a3 to e3076eb Compare December 7, 2022 11:31
@michnovka
Copy link
Contributor Author

@greg0ire

In your test, all 3 entities have the enum appear in the primary key, is that required to reproduce the issue?

It is not required, but I did like the complex structure of the example as it works through 2 levels of association instead of just one.

Enum fields as ID have worked for some time, but referencing these fields from other entities in association mappings such as OneToOne, ManyToOne and ManyToMany resulted in fatal error as there was an attempt to convert enum value to string improperly.
@michnovka
Copy link
Contributor Author

I have reduced the example entity to a minimum and provided a nice commit message.

@greg0ire please review

@greg0ire greg0ire merged commit b391431 into doctrine:2.13.x Dec 8, 2022
@greg0ire greg0ire added this to the 2.13.5 milestone Dec 8, 2022
@greg0ire greg0ire added the Bug label Dec 8, 2022
@greg0ire
Copy link
Member

greg0ire commented Dec 8, 2022

Thanks @michnovka !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enum fields can't be an id field.
3 participants