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.

Sign up for GitHub

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

Jump to bottom

Fix dbal deprecation of passing unknown options to a Column #446

Merged
merged 1 commit into from
Oct 23, 2021
Merged

Fix dbal deprecation of passing unknown options to a Column #446

merged 1 commit into from
Oct 23, 2021

Conversation

simonberger
Copy link
Contributor

@simonberger simonberger commented Oct 16, 2021 •
edited by jordisala1991
Loading

After allowing Dbal 3 in #444 several tests failed because unknown option had been passed to a Column. This was deprecated at some point.

Changelog

### Fixed
- Avoid passing unknown options to a column during its creation.

@simonberger
Copy link
Contributor Author

simonberger commented Oct 16, 2021 •
edited
Loading

I am not very happy with this fix. Imo the toArray method should not be used or should pass the platformOptions and customSchemaOptions in a sub array instead of merging them on the root level.

Another solution could be enabling the Table in dbal to add a column from another column to not need so much class knowledge in other libraries.

The parameter name options confused me most, aren't it somewhat just the properties of the column-class?

@VincentLanglet VincentLanglet mentioned this pull request Oct 17, 2021
Merged
@simonberger simonberger mentioned this pull request Oct 17, 2021
Closed
@simonberger simonberger force-pushed the fix_column_unknown_options branch 2 times, most recently from 4431d80 to 8158a77 Compare October 22, 2021 20:30
@jordisala1991
Copy link
Member

To me this solution looks great, can you fix php cs fixer build?

@jordisala1991 jordisala1991 requested review from phansys and a team October 22, 2021 21:06
phansys
phansys reviewed Oct 22, 2021
View reviewed changes
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

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

IMO, this PR is RTM.
Could you please use the PR template in order to provide the required info?

@jordisala1991
Copy link
Member

IMO, this PR is RTM. Could you please use the PR template in order to provide the required info?

Fixed

@jordisala1991 jordisala1991 merged commit 4144438 into sonata-project:1.x Oct 23, 2021
@jordisala1991
Copy link
Member

Thanks @simonberger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phansys phansys phansys approved these changes

@jordisala1991 jordisala1991 jordisala1991 approved these changes

Assignees
No one assigned
Labels
patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simonberger @jordisala1991 @phansys