Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement initial port support for PostgreSQL driver #19466

Merged
merged 7 commits into from Sep 22, 2018

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Jan 26, 2018

Pull Request for Issue #18578 cc: @irbian

Summary of Changes

Add the port option to the PostgreSQL driver

Testing Instructions

Steps to reproduce the issue

Have a connection on a port different than the default and try to make a query on it

        $options = array();
        
        $options['driver']   = 'postgresql';        
        $options['host']     = '192.192.192.192';
        $options['port'] = '15432'; 
        $options['user']     = 'myuser';       
        $options['password'] = 'mypassword'; 
        $options['database'] = 'MYDATABASE';
        $options['prefix']   = '';             
        
        $dbo =  JDatabaseDriver::getInstance( $options );

Expected result

I expect to work

Actual result

"Error connecting to PGSQL database."

Open Points

@mbabker
Copy link
Contributor

mbabker commented Jan 26, 2018

FWIW the Framework handled this a bit differently, see joomla/joomla-framework#141 for the PR adding it (implementation is basically still the same now).

And no, I don't think you need the same type of detection as the MySQLi driver. That honestly seems really verbose for reasons I don't understand without getting into the commit logs.

@zero-24
Copy link
Contributor Author

zero-24 commented Jan 26, 2018

Thanks @mbabker i have just implemented the port detection from the framework! @irbian please feel free to test this one.

if (!$this->options['port'])
{
// Port is empty or not set via options, check for port annotation (:) in the host string
$tmp = substr(strstr($this->options['host'], ':'), 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do explode(':', $this->options['host']). Then $tmp[0] will be the domain and $tmp[1] will be the port if provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used the code from here: https://github.com/joomla-framework/database/blob/master/src/Postgresql/PostgresqlDriver.php so i think we should fix it in the source first can you send a PR with the needed changes?

@@ -119,7 +155,7 @@ public function connect()
$dsn .= "host={$this->options['host']} ";
}

$dsn .= "dbname={$this->options['database']} user={$this->options['user']} password={$this->options['password']}";
$dsn .= "dbname={$this->options['database']} port={$this->options['port']} user={$this->options['user']} password={$this->options['password']}";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but if you always adding port=... then you will always force connection by TCP/IP and remove option to connect by unix socket, which usual faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i'm going to move the port setting up to the host so if the host is not set we try without host &port for a socket Connection

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry took longer than expected but it is done now :)

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 28, 2018

What is holding us back from testing and merging this thing or should this just be closed as the interest is gone?

@mbabker
Copy link
Contributor

mbabker commented Jul 28, 2018

What is holding us back from testing and merging this thing

Finding the 3 people who actually use Joomla with PostgreSQL support and getting their feedback?

@alikon
Copy link
Contributor

alikon commented Jul 29, 2018

i've tested successully on not default port like 5444

@alikon
Copy link
Contributor

alikon commented Jul 29, 2018

I have tested this item ✅ successfully on de3d104


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

@twister65
Copy link
Contributor

I can not find a port number field in the System -> Global configuration -> Server -> Database Settings form.


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

@Quy
Copy link
Contributor

Quy commented Sep 19, 2018

@twister65 You can append the port to the Host eg. 192.192.192.192:15432

@twister65
Copy link
Contributor

I have tested this item ✅ successfully on 2111f2f

Tested successfully with port 5466 and postgresql driver (as well as the already implemented port with pgsql PDO driver).


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

@Quy
Copy link
Contributor

Quy commented Sep 19, 2018

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 19, 2018
@mbabker mbabker added this to the Joomla 3.9.0 milestone Sep 22, 2018
@mbabker mbabker merged commit 2b707d5 into joomla:staging Sep 22, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 22, 2018
@zero-24 zero-24 deleted the postgressqlPortSupport branch September 22, 2018 16:45
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.

None yet

7 participants