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

Add getOptions function to Column #4888

Closed
wants to merge 1 commit into from

Conversation

simonberger
Copy link
Contributor

@simonberger simonberger commented Oct 17, 2021

Q A
Type feature
BC Break no

While working on this PR sonata-project/EntityAuditBundle#446 it was hard for me to extract the options/properties of a Column-Schema.
There might be a better way achieving what is done there (copying a column from one table to another) but in the Column class we are generally able to create a new one passing its fixed options but cannot extract them again without knowing all the options the class supports.

The use of the new getOptions function makes also the composition of toArray a bit more clear imho.

Having it in 3.1 would not help for the mentioned PR tho. I fear it would be hard to argument this to be a bugfix, but I would try. 😄

@@ -90,6 +90,26 @@ public function setOptions(array $options)
return $this;
}

/**
* @return mixed[]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think adding a method the returns a mixture of untyped properties is a good idea. I'd rather get rid of all the Column methods that deal with the mixed[] type in favor of specific and well-typed ones.

Having to deal with such arrays was unnecessarily challenging during the work on #4746 (see private AbstractPlatform::columnToArray() added).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that mixed arrays are kinda bad.

To achieve the goal of the mentioned PR we could also add a addColumnFromColumn() method to Table. The general procedure copying columns from one table to another does not look to special or does it?

Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to copy the options explicitly? You should be able to get the column from one table via getColumn() and pass it to another via addColumn(). You can clone it if necessary. Or do you want to be able to change the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morozov Having the described method is exactly my pitch here. The available addColumn method does not consume a column. Just name, type and an array of "options".
I also thought about if the name should be changeable. The Table class would not be able to do this tho, because there is no public setter method.

Copy link
Member

Choose a reason for hiding this comment

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

Having a method that accepts an object makes sense to me. Otherwise, the table acts both as a column factory and as a container. It's also odd that the constructor accepts objects but the "add" method accepts individual properties.

I'm on board about transitioning from the less strict to the more strict API but I don't want to just have two methods that do seemingly the same thing. We can think of introducing a new method and deprecating the old one. In this case, the addition of the new method would be welcomed.

I also thought about if the name should be changeable.

It's currently impossible since the columns are indexed by their name in the table. If we allow changing the column name directly on a Column object, there's no way to enforce the uniqueness of the column names in the table.

@simonberger
Copy link
Contributor Author

Closed in favor of other changes to help working with this class.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants