-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[com_fields] list field with multi-select #16177
Comments
is it related to https://issues.joomla.org/tracker/joomla-cms/16153 ? This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16177. |
Isn't that the behavior of all formfields? If a single value is stored, it's stored as a string and if you have multiple values it's stored as an array of strings. That's not unique to com_fields, you have that anywhere you use JForm. Or not? |
@AlexRed I don't beleive so, since @PhilETaylor 's PR is for fixing the output of a single value to the value opposed to a comma separated value. This is still right in non-numeric values. But I am on about the rawvalue output for multiple selects being either string or array. I would suggest we adjust the list field @Bakual and set it to array if multi-select is enabled for the field in question. Thoughts? |
That would be a BC break. I'm wondering me where do you need the check you are describing in your issue description. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16177. |
@laoneo Would it though... since if you are using multiple you will need to be checking already if it's an array or not. It would only be a BC break in my eyes if we applied it to lists globally. And not just if it's a multi-select? Simple, in this scenario we are using the List for departments and if the Person is in multiple departments we select multiple, but it many cases it's a single department. So it outputs a string for the singles and an array for multiples even though the field is set as a multi-select, we use the raw value to build a filter. Whilst I can explode, clean, lower, replace space with _ to get the same result it's not ideal since we have the rawvalue field there we are using that. |
If it comes now as string even tough multiselect is enabled and we are going to change that then it is a BC break in my eyes. |
I suppose, maybe move to J4? But it doesn't make sense to return a string or an array on a multi select. It should be one or the other. Since every component using it then needs to code in a check.
…On 22 May 2017, 10:40 +0100, Allon Moritz ***@***.***>, wrote:
If it comes now as string even tough multiselect is enabled and we are going to change that then it is a BC break in my eyes.
the problem is that you will need everywhere you are using the value an if condition to determine if it is a string or an array.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub (#16177 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/ABVglv0s0ymYKGVv6U8QmtH5El2jvtFcks5r8VgWgaJpZM4Nh6Mo).
|
This value is ment to be used on an output, for example the image list is returning a string with |
If the list is set up to allow multiple, the code already is able to deal with multiple values and thus an array. It's just the count of entries which would change. I first understood that it should be always an array, also when multiple isn't selected. But that would be wrong. |
Exactly my point
…On 22 May 2017, 13:15 +0100, Thomas Hunziker ***@***.***>, wrote:
>
> If it comes now as string even tough multiselect is enabled and we are going to change that then it is a BC break in my eyes.
>
If the list is set up to allow multiple, the code already is able to deal with multiple values and thus an array. It's just the count of entries which would change.
As long as you only enforce the array when "multiple" is enabled, I think this should be fine to do.
I first understood that it should be always an array, also when multiple isn't selected. But that would be wrong.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub (#16177 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/ABVglidnu2GwsVKXB5Mc2wDbRactwN2gks5r8Xx8gaJpZM4Nh6Mo).
|
If the value attribute of a field changes within the J3 series from string to array, then it is a BC break IMO. Did I miss her something, but you want to change the value attribute of a field from string to array for multiselect lists, or? Means when I do then in my override |
He talks about the rawvalue. |
Exactly Thomas! Allon, I've kept mentioning rawvalue :). Value is very different, this outputs a readable output and doesn't need to be changed.
…On 22 May 2017, 19:37 +0100, Thomas Hunziker ***@***.***>, wrote:
He talks about the rawvalue.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub (#16177 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/ABVglk8zdWtbw1bqXTvHME21CGqs5pyUks5r8dX2gaJpZM4Nh6Mo).
|
Ah, I tough that I miss something here. Sorry for the noise. Changing the raw value to always be an array is fine IMO. |
@tonypartridge can you do a PR for this or shall I close it ? |
If this Issue get no Response, it will be closed at 22th October 2017. |
It is a valid issue that needs resolving by someone |
I'm struggling for time at the moment, will see what I can do |
We have a PR with discussion |
When using the multi select field the raw_value is an array with multiple values else if its a singluar value it's a string.
IMO it should be an array at all times, whereas at the moment to output the raw value I now need to write a check for array do this if not do this.
It would be better practice to be an array at all times given it can handle multiple values.
The text was updated successfully, but these errors were encountered: