getkirby-v2 / toolkit

This is the deprecated toolkit for Kirby v2.
http://getkirby.com
81 stars 50 forks source link

pluck() with null as split parameter does not create a proper array #187

Closed texnixe closed 8 years ago

lukasbestle commented 8 years ago

https://forum.getkirby.com/t/filters-with-forms-problem-with-pluck/4876?u=lukasbestle

lukasbestle commented 8 years ago

What exactly do you mean with "proper array"? If I use null as split parameter, I do get an array, but an array of Field objects instead of strings. I think that's intended though.

texnixe commented 8 years ago

If that's intended behavior, why then do we get a standard array when the split parameter is set to something other than null? At least that's inconsistent behavior. Also, when looping through such an array of fields, I got an empty element in the rendered output for no obvious reason.

lukasbestle commented 8 years ago

The split param converts the values of the collection items to strings because it needs to split them. If split is not set, it won't do anything and the values will continue to be the original field objects.

We could convert everything to strings, but we would need to do that in the Pages and Files objects as Collection shouldn't be "smart" here, otherwise code will break.

texnixe commented 8 years ago

Ok, I found the issue. When using the split parameter, I get an array with (in this case) 19 elements, with the split parameter set to null, the one empty value gets included in the array of Field objects, resulting in 20 elements.

lukasbestle commented 8 years ago

But isn't that as intended as well? If no splitting is required, pluck just returns the raw values. And if it is, it will return all values. An empty string is no value though.

texnixe commented 8 years ago

Intended or not, I would not want empty values in my pluck() array and I don't see why the return value should be different depending on whether or not I set a split parameter. Of course I can work around this issue by always setting a split parameter no matter if I need to split anything or not. But that somehow does not feel right.

lukasbestle commented 8 years ago

The pluck method is dumb and just does what it's supposed to. It plucks the value of every item of the collection and returns them in an array. I think that we should keep it flexible and not try to be smart about what the user wants. If they don't want empty elements, they can still filter them out themselves.

That the split option results in a completely different result isn't really inconsistent in my opinion. It's an entirely different operation after all.

lukasbestle commented 8 years ago

Hm, I'm still not quite sure about this. I think that "fixing" this in the Collection class will lead to issues in custom code. I don't think we should convert all values to strings without the $split param.

bastianallgeier commented 8 years ago

I checked this and I agree with Lukas. The pluck function does exactly, what it is supposed to do. The question really is if we should convert everything to strings or everything to fields in the pages and files collections. But I'm not sure if this is a good idea at all.

Having empty values in the array makes sense to me as well, as it just reflects, what is in each field. You can simply fix this with array_filter for example. I think I will close this for now.