? ? Pending

User tests: Successful: Unsuccessful:

avatar alikon
alikon
20 Jul 2019

Summary of Changes

use prepared statement for SQL

Testing Instructions

test com_fields

Expected result

works as before

Actual result

N/A

avatar alikon alikon - open - 20 Jul 2019
avatar alikon alikon - change - 20 Jul 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 20 Jul 2019
Category Administration com_fields
avatar alikon alikon - change - 20 Jul 2019
Labels Added: ?
3844f1e 21 Jul 2019 avatar alikon cs
avatar Quy Quy - change - 21 Jul 2019
Title
[com_fields] convert to prepared sql
[4.0][com_fields] convert to prepared sql
avatar Quy Quy - edited - 21 Jul 2019
f74e2c5 28 Aug 2019 avatar alikon oops
d9212ef 29 Aug 2019 avatar alikon cs
avatar Quy
Quy - comment - 20 Sep 2019

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
avatar alikon
alikon - comment - 22 Sep 2019

should be fixed now

avatar wilsonge
wilsonge - comment - 15 Nov 2019

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

avatar alikon
alikon - comment - 15 Nov 2019

i'll look asap

avatar alikon
alikon - comment - 15 Nov 2019

@wilsonge done

avatar Quy Quy - test_item - 7 Jan 2020 - Tested successfully
avatar Quy
Quy - comment - 7 Jan 2020

I have tested this item successfully on b1b76f6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25657.

avatar richard67
richard67 - comment - 7 Jan 2020

@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.

avatar alikon
alikon - comment - 7 Jan 2020

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

avatar alikon
alikon - comment - 7 Jan 2020

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

avatar richard67
richard67 - comment - 7 Jan 2020

@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.

avatar richard67
richard67 - comment - 7 Jan 2020

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.

avatar Quy
Quy - comment - 7 Jan 2020

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

avatar richard67
richard67 - comment - 7 Jan 2020

@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.

avatar SharkyKZ
SharkyKZ - comment - 7 Jan 2020

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.

avatar richard67
richard67 - comment - 7 Jan 2020

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.

avatar jwaisner jwaisner - test_item - 8 Jan 2020 - Tested successfully
avatar jwaisner
jwaisner - comment - 8 Jan 2020

I have tested this item successfully on b1b76f6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25657.

avatar alikon alikon - change - 8 Jan 2020
Status Pending Ready to Commit
avatar alikon
alikon - comment - 8 Jan 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25657.

avatar HLeithner HLeithner - close - 8 Jan 2020
avatar HLeithner HLeithner - merge - 8 Jan 2020
avatar HLeithner HLeithner - change - 8 Jan 2020
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: ?
avatar HLeithner
HLeithner - comment - 8 Jan 2020

Thanks

Add a Comment

Login with GitHub to post a comment