Pg schema #24999

softarius · 2019-05-24T09:11:40Z

Pull Request for Issue #24998 .

Summary of Changes

Method getTableColumns of JDatabaseDriverPostgresql class

Testing Instructions

install Joomla with PostgreSQL driver.
create a new schema (for example 'test')
create a new table in this schema

CREATE TABLE test.customer (
  id SERIAL,
  name VARCHAR(100),
  PRIMARY KEY(id)
) 
WITH (oids = false);

in code call getTableColumns for this table

$db = JFactory::getDBO();
$custfields=$db->getTableColumns('test.customer');

Expected result

Array with fields of a table

Actual result

Empty array

Documentation Changes Required

Added to param $table
For PostgreSQL may starting with a schema

This reverts commit 654cc87.

This reverts commit fcb8c91.

richard67 · 2019-05-24T09:20:27Z

@alikon Do you think this is a new feature for 4.0, or is it a bug fix for 3.9?

alikon · 2019-05-24T09:23:49Z

quite a greyzone it dependes on how you feel that, so up to the Production Lead

richard67 · 2019-05-24T09:25:18Z

schema.php

@@ -0,0 +1,23 @@
+<?php


Please remove this file from your PR since it is only for testing this PR, so it should not be checked into the Joomla CMS source code. You have that code already in the testing instructions.

It would be better to convert it into a real test and integrated it into the test suite.

Sure, that would be best. If the author of this PR feels not being able to do that, we can do that later with a new PR or help him here. But anyway this files as it is should be removed from this PR.

Now file 'schema.php' simply deleted

richard67 · 2019-05-24T09:26:20Z

libraries/joomla/database/driver/postgresql.php

@@ -406,6 +406,11 @@ public function getTableColumns($table, $typeOnly = true)
 		$result = array();

 		$tableSub = $this->replacePrefix($table);
+	        $fn = explode('.', $tableSub);


Code style error reported by Drone for lines 409 to 413: Tabs must be used to indent lines; spaces are not allowed

richard67 · 2019-05-24T09:31:20Z

libraries/joomla/database/driver/postgresql.php

@@ -406,6 +406,11 @@ public function getTableColumns($table, $typeOnly = true)
 		$result = array();

 		$tableSub = $this->replacePrefix($table);
+	        $fn = explode('.', $tableSub);
+        	if (count($fn) == 2) {


Mind the Joomla CMS code style rules or check the Drone report.
An "if" with "else" should look as follows, where "..." means some code:

if (...)
{
...
}
else
{
...
}

Please correct that to make Drone happy.

P.S. And indent with tabs

richard67 · 2019-05-24T09:38:44Z

@HLeithner Do you think this PR is a bug fix for 3.9 or a new feature for 4.0? From my point of view it is a bug fix since it is not good practice to use the public schema for everything in PostgreSQL, as far as I know, like it has to be without this PR. But I might be wrong, so I'd like to know your opinion, and @alikon 's, too, because he is doing much with PostgreSQL.

HLeithner · 2019-05-24T09:46:04Z

libraries/joomla/database/driver/postgresql.php

@@ -427,7 +432,7 @@ public function getTableColumns($table, $typeOnly = true)
 			WHERE a.attrelid =
 				(SELECT oid FROM pg_catalog.pg_class WHERE relname=' . $this->quote($tableSub) . '
 					AND relnamespace = (SELECT oid FROM pg_catalog.pg_namespace WHERE
-					nspname = \'public\')
+					nspname = \'' . $schema . '\')


Use $this->quote() for quoting please.

HLeithner · 2019-05-24T09:46:23Z

@HLeithner Do you think this PR is a bug fix for 3.9 or a new feature for 4.0? From my point of view it is a bug fix since it is not good practice to use the public schema for everything in PostgreSQL, as far as I know, like it has to be without this PR. But I might be wrong, so I'd like to know your opinion, and @alikon 's, too, because he is doing much with PostgreSQL.

It's a bugfix.

alikon · 2019-05-24T09:51:22Z

to me too it's a bugfix

ghost · 2019-05-24T09:58:23Z

@pviolette-fr please test > https://docs.joomla.org/Testing_Joomla!_patches

…g-schema

…nto pg-schema

softarius · 2019-05-24T11:08:46Z

@HLeithner Do you think this PR is a bug fix for 3.9 or a new feature for 4.0? From my point of view it is a bug fix since it is not good practice to use the public schema for everything in PostgreSQL, as far as I know, like it has to be without this PR. But I might be wrong, so I'd like to know your opinion, and @alikon 's, too, because he is doing much with PostgreSQL.

My PR isn't only bug-fix. It's a feature request for a future version of Joomla.
Best practice for PostgresSQL is using different schemas for logical subsystems into a database.
Proof - https://www.postgresql.org/docs/10/ddl-schemas.html#DDL-SCHEMAS-PATTERNS
Simple RDBS (not supported schemas, such as MySQL) use surrogate solving with a prefix for tables.
When I write a new component for Joomla with PostgreSQL, I wont to use for this separate schema.
In another side, in big and complex system Joomla could use schema. called other than "public". But Joomla can't do it.
Third, with good schemas supporting Joomla could switching between schemas just different databases.

HLeithner · 2019-05-24T11:18:24Z

I'm not sure what you mean with "Simple RDBS (not supported schemas, such as MySQL)" is the schema support of MySQL different then the support form PostgresSQL?

softarius · 2019-05-24T12:23:05Z

I'm not sure what you mean with "Simple RDBS (not supported schemas, such as MySQL)" is the schema support of MySQL different then the support form PostgresSQL?

Using the term 'schema' completely different in other systems.
In MySQL and Firebird command CREATE SCHEMA is a synonym for CREATE DATABASE -

In more powerful RDBMS (Oracle, SQL Server, PostgreSQL ) a schema is essentially a namespace.
Proofs:

Quy · 2019-05-24T12:26:25Z

libraries/joomla/database/driver/postgresql.php

 		$tableSub = $this->replacePrefix($table);
+		$fn = explode('.', $tableSub);
+		if (count($fn) == 2) 


Suggested change

if (count($fn) == 2)

if (count($fn) == 2)

Quy · 2019-05-24T12:27:36Z

libraries/joomla/database/driver/postgresql.php

+		$fn = explode('.', $tableSub);
+		if (count($fn) == 2) 
+		{
+		$schema = $fn[0];


Indent lines 411, 412, and 416.

Quy · 2019-05-24T12:28:18Z

libraries/joomla/database/driver/postgresql.php

@@ -391,7 +391,7 @@ public function getTableCreate($tables)
 	/**
 	 * Retrieves field information about a given table.
 	 *
-	 * @param   string   $table     The name of the database table.
+	 * @param   string   $table     The name of the database table. For PostgreSQL may starting with a schema


Suggested change

* @param string $table The name of the database table. For PostgreSQL may starting with a schema

* @param string $table The name of the database table. For PostgreSQL may start with a schema.

HLeithner · 2019-05-24T12:35:43Z

I'm not sure what you mean with "Simple RDBS (not supported schemas, such as MySQL)" is the schema support of MySQL different then the support form PostgresSQL?

Using the term 'schema' completely different in other systems.
In MySQL and Firebird command CREATE SCHEMA is a synonym for CREATE DATABASE -

ok thx, anyway in this case it's only a bugfix because it's should handle this and don't break.

alikon · 2019-05-24T17:04:45Z

I have tested this item ✅ successfully on d16b754

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24999.}

richard67 · 2019-05-25T10:00:58Z

@softarius I've changed the "Actual result" in your testing instructions to the value from the issue. I guess that was a copy and paste thing from "Expected result", so it was the same as that, and in the issue it was correct, right?

richard67 · 2019-05-25T10:02:05Z

@HLeithner I can fully confirm @softarius 's explanations above.

richard67 · 2019-05-25T10:32:35Z

I have tested this item ✅ successfully on d16b754

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24999.}

alikon · 2019-05-25T10:33:43Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24999.}

richard67 · 2019-05-25T10:35:24Z

@alikon Have there ever been discussions in past about full db schema support in Joomla, i.e. you can specify a schema like you can specify user and database names when installing on PostgreSQL, so Joomla can be installed using a different schema than "Public"? If so, what was the result of these discussions?

richard67 · 2019-05-25T10:36:36Z

For the one who will merge: Drone failure is not related to this PR.

softarius · 2019-05-25T19:20:10Z

@softarius I've changed the "Actual result" in your testing instructions to the value from the issue. I guess that was a copy and paste thing from "Expected result", so it was the same as that, and in the issue it was correct, right?

Yes.

libraries/joomla/database/driver/postgresql.php

alikon · 2019-05-27T06:41:36Z

@richard67

Have there ever been discussions in past about full db schema support in Joomla

not that i'm aware of

Co-Authored-By: SharkyKZ <sharkykz@gmail.com>

richard67 · 2019-05-30T09:12:46Z

I have tested this item ✅ successfully on 3da16d1

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24999.}

HLeithner · 2019-06-05T15:46:59Z

thx

softarius and others added 5 commits May 23, 2019 22:06

Revert "Move SECURITY.md to the .github folder (#24994)"

fcb8c91

This reverts commit 654cc87.

Revert "Revert "Move SECURITY.md to the .github folder (#24994)""

fc81a11

This reverts commit fcb8c91.

getTableColumns for another schema

845335b

Pg-schema test

afac9aa

Comment for table name with schema

dbbf944

joomla-cms-bot added the PR-staging label May 24, 2019

softarius mentioned this pull request May 24, 2019

Getting fields for a table in another schema #24998

Closed

Merge branch 'staging' into pg-schema

bed083c

richard67 requested changes May 24, 2019

View reviewed changes

HLeithner reviewed May 24, 2019

View reviewed changes

alikon mentioned this pull request May 24, 2019

Postgresql can't use JTable with table not in public schema #20234

Closed

softarius added 4 commits May 24, 2019 13:11

schema.php deleted

7209008

Merge branch 'staging' of https://github.com/joomla/joomla-cms into p…

c091952

…g-schema

Merge branch 'pg-schema' of https://github.com/softarius/joomla-cms i…

aaaae81

…nto pg-schema

Code style fixed

239b873

Quy reviewed May 24, 2019

View reviewed changes

softarius added 2 commits May 24, 2019 15:33

Idents

94621a1

Dot in comment

fc32c67

softarius added 2 commits May 24, 2019 15:38

New line befor if

37297fe

Quoted

d16b754

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 25, 2019

SharkyKZ reviewed May 26, 2019

View reviewed changes

libraries/joomla/database/driver/postgresql.php Outdated Show resolved Hide resolved

Update libraries/joomla/database/driver/postgresql.php

3da16d1

Co-Authored-By: SharkyKZ <sharkykz@gmail.com>

HLeithner merged commit 4db621f into joomla:staging Jun 5, 2019

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 5, 2019

HLeithner added this to the Joomla 3.9.7 milestone Jun 5, 2019

softarius deleted the pg-schema branch June 6, 2019 16:24

wilsonge mentioned this pull request Aug 31, 2019

Port required from Joomla CMS joomla-framework/database#157

Open

richard67 mentioned this pull request Nov 24, 2020

Allow other schema than "public" on PostgreSQL databases joomla-framework/database#217

Merged

	* @param string $table The name of the database table. For PostgreSQL may starting with a schema
	* @param string $table The name of the database table. For PostgreSQL may start with a schema.

Pg schema #24999

Pg schema #24999

Conversation

softarius commented May 24, 2019 • edited by joomla-cms-bot Loading

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

richard67 commented May 24, 2019

alikon commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richard67 May 24, 2019 • edited Loading

Choose a reason for hiding this comment

richard67 May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richard67 commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HLeithner commented May 24, 2019

alikon commented May 24, 2019

ghost commented May 24, 2019

softarius commented May 24, 2019

HLeithner commented May 24, 2019

softarius commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HLeithner commented May 24, 2019

alikon commented May 24, 2019

richard67 commented May 25, 2019

richard67 commented May 25, 2019

richard67 commented May 25, 2019

alikon commented May 25, 2019

richard67 commented May 25, 2019

richard67 commented May 25, 2019

softarius commented May 25, 2019

alikon commented May 27, 2019

richard67 commented May 30, 2019

HLeithner commented Jun 5, 2019

softarius commented May 24, 2019 •

edited by joomla-cms-bot

Loading

richard67 May 24, 2019 •

edited

Loading

richard67 May 24, 2019 •

edited

Loading