User tests: Successful: Unsuccessful:
use prepared statement for SQL
test com_fields
works as before
N/A
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_fields |
Labels |
Added:
?
|
Title |
|
should be fixed now
i'll look asap
I have tested this item
@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:
... OR ...
syntax everywhere in that function and not extendWhere
.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?
adding a piece of magic like
->where('1=1')
before any instruction ofextendWhere
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.
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
Status | Pending | ⇒ | Ready to Commit |
RTC
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-01-08 09:22:42 |
Closed_By | ⇒ | HLeithner | |
Labels |
Added:
?
|
Thanks
Under Fields, Field Group column does not sort.
To reproduce: