Skip to content

[4.0][com_fields] convert to prepared sql#25657

Merged
HLeithner merged 51 commits intojoomla:4.0-devfrom
alikon:patch-116
Jan 8, 2020
Merged

[4.0][com_fields] convert to prepared sql#25657
HLeithner merged 51 commits intojoomla:4.0-devfrom
alikon:patch-116

Conversation

@alikon
Copy link
Copy Markdown
Contributor

@alikon alikon commented Jul 20, 2019

Summary of Changes

use prepared statement for SQL

Testing Instructions

test com_fields

Expected result

works as before

Actual result

N/A

Co-Authored-By: SharkyKZ <sharkykz@gmail.com>
@Quy Quy changed the title [com_fields] convert to prepared sql [4.0][com_fields] convert to prepared sql Jul 21, 2019
@Quy
Copy link
Copy Markdown
Contributor

Quy commented Jan 7, 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.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jan 7, 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.

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented Jan 7, 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

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented Jan 7, 2020

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

@richard67
Copy link
Copy Markdown
Member

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

@richard67
Copy link
Copy Markdown
Member

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.

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Jan 7, 2020

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

@richard67
Copy link
Copy Markdown
Member

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

@SharkyKZ
Copy link
Copy Markdown
Contributor

SharkyKZ commented Jan 7, 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.

@richard67
Copy link
Copy Markdown
Member

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.

@jwaisner
Copy link
Copy Markdown
Member

jwaisner commented Jan 8, 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.

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented Jan 8, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 8, 2020
@HLeithner HLeithner merged commit 08d91bd into joomla:4.0-dev Jan 8, 2020
@HLeithner
Copy link
Copy Markdown
Member

Thanks

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 8, 2020
@HLeithner HLeithner added this to the Joomla 4.0 milestone Jan 8, 2020
@alikon alikon deleted the patch-116 branch January 8, 2020 09:23
brianteeman pushed a commit to brianteeman/joomla-cms that referenced this pull request Feb 4, 2020
Co-Authored-By: SharkyKZ <sharkykz@gmail.com>
Co-Authored-By: Quy <quy@fluxbb.org>
Co-authored-by: George Wilson <georgejameswilson@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants