Under Fields, Field Group column does not sort.

To reproduce:

• Create a field group
• Create fields and assign a field group to one of the fields
• Click Field Group header column to sort

should be fixed now

@alikon any chance of you looking at the feedback here?

i'll look asap

@wilsonge done

I have tested this item ✅ successfully on b1b76f6

@alikon In a previous review, @SharkyKZ pointed out that you can't use extendWhere if you can't be sure that there is not at least one where before it. That was in the getListQuery function in file administrator/components/com_fields/Model/FieldsModel.php. In a later review @Quy told to use extendWhere at some place in the same function and file, and you followed that, so my fear is that the previous valid change due to @SharkyKZ was changed back.

Furthermore, I've just checked the getListQuery getListQuery function in file administrator/components/com_fields/Model/FieldsModel.php from top to bottom, and I noticed that every where or extendWhere is inside an if condition for a filter, i.e. if none of the first few conditions is true and then one with an extendWhere is true, we have the case of extendWhere without previous where.

There are 2 alternative ways to solve it:

• Use traditional ... OR ... syntax everywhere in that function and not extendWhere.
• Add a where at the top which always is true, e.g. ->where('1=1'), at the top to the query, so any later extendWhere added depending on any filter condition will have a previous where.

Please @Quy and @SharkyKZ discuss that on Glip, if necessary, so that there is common sense between you both for future reviews.

And please @wilsonge take a note of this comment here and the issue it describes. I think we might have this problem in other list queries, where depending on the combination of filter criteria there might be some extendWhere without a previous where, and let me know which of the 2 possible solutions I desribed above is preferred by you in case if it turns out that I am right and we really have that problem at more places.

part of me have liked too much the "black magic art" like ->where('1=1')
anyway, you've raised a valid concern
let's discuss it proofly in jbs meeting or whatever

adding a piece of magic like ->where('1=1') before any instruction of extendWhere code
sounds ugly? ?

@alikon Problem when using the other possible solution, traditional syntax, in a few months someone will come and make a PR to change them all to extendWhere ;-)

Sure we can discuss that on Friday.

adding a piece of magic like ->where('1=1') before any instruction of extendWhere code
sounds ugly? ?

Doesn't sound ugly to me if done only one time at the top of the query and not before every extendWhere. I'd even prefer this solution for the reason mentioned in my previous comment.

@richard67 Each extendWhere has a where or whereIn before it so I don't see where the issue is.

@Quy Oh, you are right. In this PR here it's ok. Sorry for the rumor. But I still think we should be aware that it might be a problem elsewhere, or become a problem if someone wants to change "classical" syntax to extendWhere at critical places in future PRs.

Maybe extendWhere() can be improved not to require a where() clause. Or another method introduced if extendWhere() was not intended to be used like this.

So or so sorry for rumors. Here in this PR it is ok now. Is too late here now to start, but I have to find time soon to test this PR. I'll try latest on weekend.

I have tested this item ✅ successfully on b1b76f6

RTC

Thanks