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

Convert PHP to SQL for new object expression #8062

Merged
merged 1 commit into from Apr 16, 2020
Merged

Convert PHP to SQL for new object expression #8062

merged 1 commit into from Apr 16, 2020

Conversation

jeroenvdheuvel
Copy link
Contributor

Fields used in the new object expression. Were not yet converted to via
their configured type/convertToPHPValueSQL. This can lead to incorrect
arguments being injected into the new object.

This is fixed by calling convertToPHPValueSQL while creating the SQL
query.

This fixes issue #8061

Fields used in the new object expression. Were not yet converted to via
their configured type/convertToPHPValueSQL. This can lead to incorrect
arguments being injected into the new object.

This is fixed by calling convertToPHPValueSQL while creating the SQL
query.
@SenseException
Copy link
Member

Thank you for your addition to the NEW object syntax. The implementation looks good, but I'm not sure if this belongs into the 2.7 branch. This looks more like an improvement than a bugfix. @doctrine/team-orm

@ostrolucky
Copy link
Member

Hard to say. I'm ok with both.

@jeroenvdheuvel
Copy link
Contributor Author

jeroenvdheuvel commented Mar 12, 2020

It might be both. I considered it a bug because for me this is unexpected behavior. convertToPHPValueSQL is called when hydrating an entity or an array but not when creating a new object (the odd one out).

The custom types mapping documentation also doesn't mention anything specific about not supporting the new object syntax.

@jeroenvdheuvel
Copy link
Contributor Author

@SenseException or @ostrolucky what should I do to get this changed merged?

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

If 2.7 is the right branch for this, I'm 👍 with it.

@ostrolucky ostrolucky merged commit 73ec483 into doctrine:2.7 Apr 16, 2020
@ostrolucky ostrolucky added this to the 2.7.3 milestone Apr 16, 2020
@jeroenvdheuvel
Copy link
Contributor Author

Thanks @ostrolucky and @SenseException !

@jeroenvdheuvel jeroenvdheuvel deleted the GH8061 branch April 20, 2020 13:23
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

3 participants