? ? Pending

User tests: Successful: Unsuccessful:

avatar alikon
alikon
16 Aug 2019

Summary of Changes

wrong encoding for bytea
wrong inner join update syntax

Testing Instructions

on postgresql
enable smart search plugin
edit & save a tag

Expected result

tag is saved
works as before on mysql

Actual result

multiple errors

avatar alikon alikon - open - 16 Aug 2019
avatar alikon alikon - change - 16 Aug 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 Aug 2019
Category Administration com_finder
avatar HLeithner
HLeithner - comment - 16 Aug 2019

All new pr should use prepared statements, could you update this pr?

avatar alikon
alikon - comment - 16 Aug 2019

well yes, i could, but atm with this silly PR i just want to give to the few postgresql users the opportunity to play a little bit with j4, and help us to discover more bugs.....prepared statement conversion should be matter for another pr imho

avatar HLeithner
HLeithner - comment - 16 Aug 2019

At the moment only you and me did prepared statement conversion it should be a strict requirement on all pr touching a sql statement to convert it to use prepared statements.
There was a PLT motion about this topic:

https://volunteers.joomla.org/departments/production/reports/1013-production-dept-meeting-may-07-and-may-14-2019

If we don't required this we will still get new non prepared queries in patches.

avatar alikon
alikon - comment - 16 Aug 2019

i'd be more than happy if this motion have been taken in serious count before merging all & more,
but honestly this has not been happened....

avatar mbabker
mbabker - comment - 16 Aug 2019

At the moment only you and me did prepared statement conversion it should be a strict requirement on all pr touching a sql statement to convert it to use prepared statements.

Forcing unrelated changes into pull requests is exactly how you end up making pull requests more complex to review and more error prone. Converting existing queries should be pull requests of their own, writing new queries it is fine to mandate this. "You touched it so you need to change it" isn't a good workflow. Same goes with the random unrelated code style fixes that are commonly requested.

avatar HLeithner
HLeithner - comment - 16 Aug 2019

Forcing unrelated changes into pull requests is exactly how you end up making pull requests more complex to review and more error prone. Converting existing queries should be pull requests of their own, writing new queries it is fine to mandate this. "You touched it so you need to change it" isn't a good workflow. Same goes with the random unrelated code style fixes that are commonly requested.

This is partly true, but whats the better way? If no queries get updated to PS only if we find more then 2 people to do this job we will never finish this task.

Anyway I don't request to update all queries in this file only the one that get touched and tested afterwards. In a perfect would we would have tests at the same time, or the PR includes a test if non exists but as we all know tests are still missing all over the place.

It's ok for me if at least new queries use PS...

avatar richard67
richard67 - comment - 17 Aug 2019

@alikon Will you work on this, make Harald happy with just changing those queries to PS which you have modified, and handle Hannes‘ comments? If so, ping me when ready for tests. But if you have little time: Shall I help with picking up that work and making PR for your repo so you just can merge with 1 click? If so, ping me and I will do later today.

avatar alikon alikon - change - 18 Aug 2019
Labels Added: ?
d709e32 18 Aug 2019 avatar alikon cs
avatar richard67
richard67 - comment - 18 Aug 2019

@Hackwar Could you check if it ok now?

avatar twister65 twister65 - test_item - 15 Sep 2019 - Tested successfully
avatar twister65
twister65 - comment - 15 Sep 2019

I have tested this item successfully on c484f45


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

avatar twister65
twister65 - comment - 15 Sep 2019

I have tested this item successfully on c484f45


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

avatar richard67 richard67 - test_item - 15 Sep 2019 - Tested successfully
avatar richard67
richard67 - comment - 15 Sep 2019

I have tested this item successfully on c484f45


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

avatar richard67
richard67 - comment - 15 Sep 2019

@Quy RTC please ?

avatar Quy Quy - change - 15 Sep 2019
Status Pending Ready to Commit
avatar Quy
Quy - comment - 15 Sep 2019

RTC


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

avatar alikon alikon - change - 17 Sep 2019
Labels Added: ?
avatar wilsonge wilsonge - change - 17 Sep 2019
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-09-17 08:47:35
Closed_By wilsonge
avatar wilsonge wilsonge - close - 17 Sep 2019
avatar wilsonge wilsonge - merge - 17 Sep 2019
avatar wilsonge
wilsonge - comment - 17 Sep 2019

Thanks!

Add a Comment

Login with GitHub to post a comment