? ? ? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
24 Mar 2020

Summary of Changes

The install file for postgres is invalid, single quotes are not escaped properly.

It is also possible to escape it with (E'i\'m', 'en', 0),. What you guys prefer.

Testing Instructions

Install joomla with postgres.

Expected result

Installation goes through.

Actual result

Installation fails.

avatar laoneo laoneo - open - 24 Mar 2020
avatar laoneo laoneo - change - 24 Mar 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Mar 2020
Category Postgresql SQL Installation
avatar laoneo laoneo - change - 24 Mar 2020
The description was changed
avatar laoneo laoneo - edited - 24 Mar 2020
avatar richard67
richard67 - comment - 24 Mar 2020

@laoneo Please do the same changes in the update sql script postgresql/4.0.0-2018-07-29.sql, too.

avatar richard67
richard67 - comment - 24 Mar 2020

@laoneo Regarding your testing instructions: Here with PostgreSQL 11 installation doesn't fail without this PR, it just gives a lot of warnings in the log file of the postgresql server.

avatar richard67
richard67 - comment - 24 Mar 2020

@alikon What do you prefer for PostgreSQL? Escape E'i\'m' or 2 single quotes 'i''m' for string literal containing a single quote?

avatar richard67
richard67 - comment - 24 Mar 2020

@laoneo P.S. regarding update sql script postgresql/4.0.0-2018-07-29.sql: We can safely update it here because we don't support updates of 4 versions before beta.

avatar laoneo laoneo - change - 24 Mar 2020
Labels Added: ? ?
avatar laoneo
laoneo - comment - 24 Mar 2020

Update file done.

avatar joomla-cms-bot joomla-cms-bot - change - 24 Mar 2020
Category Postgresql SQL Installation Postgresql SQL Administration com_admin Installation
avatar alikon
alikon - comment - 24 Mar 2020

well if it is a matter of what i prefer than my preference goes to 'i''m'
i think is more easily readble

avatar richard67
richard67 - comment - 24 Mar 2020

Has both its advantages and disadvantages when doing a comparison between the joomla.sql files of the two database types to check if data is the same. Either you see an "E" at the beginning of a string or the additional single quote instead of a backslash in the middle of a string in the differences.

avatar richard67
richard67 - comment - 24 Mar 2020

I've just verified that we don't have the single quotes within strings elsewhere in sql, so we are free in our choice and don't need to be consistent with elsewhere, and I can confirm that this PR covers all occurrences.

avatar richard67
richard67 - comment - 24 Mar 2020

I have tested this item successfully on 59791d6

Hint for other testers: Different to what is described in section "Actual result", the installation worked here on PostgreSQL for me without the PR.

But there were lots of log entries like WARNING: nonstandard use of \' in a string literal at character ... in the postgresql server log file, and these disappear when having this PR applied.

There are many log entries like WARNING: nonstandard use of \\ in a string literal at character ... left in the postgresql server log file, so it seems we have elsewhere a problem with quoted backslashes in our sql. I would assume it is class paths in extensions table ;-) @laoneo Could you check that and if possible correct with another PR?


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

avatar richard67 richard67 - test_item - 24 Mar 2020 - Tested successfully
avatar richard67
richard67 - comment - 24 Mar 2020

@laoneo For the other issue with quoting backslash which I've mentioned in my testing result: Do you want to make a PR for that, or shall I do that?

avatar laoneo
laoneo - comment - 24 Mar 2020

Can you?

avatar richard67
richard67 - comment - 24 Mar 2020

I'll try, be patient.

avatar richard67
richard67 - comment - 24 Mar 2020

@laoneo It seems fixing the double backslash warnings would require to use the E'...' syntax. To be consistent then this PR would be better if using that syntax, too. But I am not 100% sure about that yet.

avatar richard67
richard67 - comment - 24 Mar 2020

Forget my previous post, this PR here is good as it is.

avatar alikon
alikon - comment - 25 Mar 2020

@laoneo add change for finder.sql file see Digital-Peak#14

avatar joomla-cms-bot joomla-cms-bot - change - 25 Mar 2020
Category Postgresql SQL Installation Administration com_admin Postgresql SQL Administration com_admin com_finder Installation
avatar alikon
alikon - comment - 25 Mar 2020

I have tested this item successfully on 00e8113

tested on postgresql 11


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

avatar alikon alikon - test_item - 25 Mar 2020 - Tested successfully
avatar alikon
alikon - comment - 25 Mar 2020

we still have warning for the double/triple backslash, but matter for another pr

avatar richard67
richard67 - comment - 25 Mar 2020

I have tested this item successfully on 00e8113

Tested on PotsgreSQL 11.

The last change (file administrator/components/com_finder/sql/install.postgresql.sql) I've tested by review.

From my point of view this file is not really needed. Or is there a scenario in which com_finder has to be (re-)installed (normal install or discovery)? @wilsonge what do you think?


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

avatar richard67 richard67 - test_item - 25 Mar 2020 - Tested successfully
avatar richard67 richard67 - change - 25 Mar 2020
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 25 Mar 2020

RTC


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

avatar richard67
richard67 - comment - 25 Mar 2020

@alikon For the (double) backslashes I have prepared a branch for a PR, using the E'...' syntax for those strings which contain \/ or \\. You could see the changes here: 4.0-dev...richard67:4.0-dev-fix-double-backslash-warnings-postgresql.

That removes almost all of the warnings from the postgresql-11.log, but a few remain of which I haven't found the source yet. Maybe these come from database actions in PHP? If so, we would have a problem if we don't want to have database-specific statements (what would be a mess). So maybe we should set a session variable standard_conforming_strings=on? The changes in this PR here will still be correct then, but for the other backslashes we might be ok then without any other change? Can you check that? If necessary we can talk on Glip in the evenings or on weekend.

avatar alikon
alikon - comment - 25 Mar 2020

i was playing on the same toy as you ?
and i've the same conclusion as you

but a few remain of which I haven't found the source yet.

Yeah better to chat on Glip ......

avatar wilsonge wilsonge - change - 25 Mar 2020
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-03-25 10:49:00
Closed_By wilsonge
Labels Added: ?
avatar wilsonge wilsonge - close - 25 Mar 2020
avatar wilsonge wilsonge - merge - 25 Mar 2020
avatar wilsonge
wilsonge - comment - 25 Mar 2020

Thanks!

Add a Comment

Login with GitHub to post a comment