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

[5.3] Feature Request – Support non-standard database ports #43902

Open
muhme opened this issue Aug 10, 2024 · 19 comments
Open

[5.3] Feature Request – Support non-standard database ports #43902

muhme opened this issue Aug 10, 2024 · 19 comments

Comments

@muhme
Copy link
Contributor

muhme commented Aug 10, 2024

Last edited on 30 October 2024 to update the list of PRs implementing this issue

Is your feature request related to a problem? Please describe.

To be able to enter non-standard port numbers for all supported database servers in the Web Installer and using it in Cypress System Tests.

This is currently possible for MySQL and MariaDB, but not for PostgreSQL. See screenshots:
mysql
mariadb
postgres
error

Describe the solution you'd like

Be able to enter host:port (using hostname, IPv4 address or IPv6 address) in the Web Installer for all supported databases and databases types::

  • MySQL with MySQL (PDO) and MySQLi
  • MariaDB with MySQL (PDO) and MySQLi
  • PostgreSQL with PostgreSQL (PDO)

And adopting the System Tests to be able to handle non-standard database port numbers.

Issues Implementing this Feature Request

Open: To document the feature.

@richard67
Copy link
Member

When reading my old PRs which fixed the database host check for port numbers in the host name it seems to me that it once was working for PostgreSQL: #29567 , #29568 .

@muhme
Copy link
Contributor Author

muhme commented Aug 11, 2024

When reading my old PRs which fixed the database host check for port numbers in the host name it seems to me that it once was working for PostgreSQL: #29567 , #29568 .

Thank you for checking and adding the PRs 👍 And it looks to me as if the test instructions demand that PostgreSQL should be tested, but it was not tested: "no PostgreSQL".

@richard67
Copy link
Member

And it looks to me as if the test instructions demand that PostgreSQL should be tested, but it was not tested: "no PostgreSQL".

I would assume that I had tested it myself on PostgreSQL, but I don’t remember it now.

@richard67
Copy link
Member

@muhme What happens if you use the "MySQL (PDO)" driver with MySQL or MariaDB and not the "MySQLi"?

And when you have tested PostgreSQL (PDO), was the database server listing at that port? If not: Could you try again with the PostgreSQL database server being configured to work on the custom port used for the test?

When you enter database connection data and then continue, the connection is tested by the installation. It might be that this works slightly different with the PDO and the MySQLi drivers, so that it fails with that error message when the connection to that port doesn't work.

Could you check and report back if that is the case?

@muhme
Copy link
Contributor Author

muhme commented Aug 11, 2024

What happens if you use the "MySQL (PDO)" driver with MySQL or MariaDB and not the "MySQLi"?

Successful tested host.docker.internal:7011 is also working for "MySQL (PDO)" with MySQL and host.docker.internal:7012 for MariaDB. Added as requirement to the solution, good point.

was the database server listing at that port?

Yes, tested database host:port is reachable from web server container. If not, it takes a long time until the timeout. I assume, failing here is the name resolution as host:port together are used as hostname.

PostgreSQL database server being configured to work on the custom port used for the test?

No, inside the docker container PostgreSQL is running on port 5432. Or do you mean using the standard port :5432, yes this was tried and is also not working. Or do you mean reaching the database from web server, yes from web server telnet jbt_pg 5432 can establish a connection.

I guess host:port is used as hostname and not working for PostgreSQL driver. I saw samples like pg_connect("host=sheep port=5432 dbname=mary user=lamb password=foo"); so port may have to be given in different way.

@richard67
Copy link
Member

No, inside the docker container PostgreSQL is running on port 5432.

@muhme What I still don't understand: For the others you have used host.docker.internal:xyz, with xyz being the custom port. For PostgreSQL your screenshot shows localhost. So is the webserver running in the same container as the database server or in a different container? A localhost in the installation form will resolve to the webserver.

@muhme
Copy link
Contributor Author

muhme commented Aug 11, 2024

I tried a lot and picked up the screenshot with localhost as localhost is always reachable. Currently double checked with container hostname and with mapped port:

could not translate host name "jbt_pg:5432" to address: Name or service not known
could not translate host name "host.docker.internal:7013" to address: Name or service not known

Both fail and the two analogue configurations work for MySQL and MariaDB. But anyway, thank you for your questions. 👍

@richard67
Copy link
Member

When using localhost the PDO driver may end up in using a unix socket, so 127.0.0.1 should be used to avoid that. See e.g. https://stackoverflow.com/questions/21046672/pdo-not-working-with-port .

Then I found out that the MySQLi driver extracts the port number from the host string here https://github.com/joomla-framework/database/blob/3.x-dev/src/Mysqli/MysqliDriver.php#L201-L252 because the MySQLi client requires the port to be specified with a separate parameter and not in the host name.

That piece of code by the way also gives some hints in comments e.g. on how to specify in IPv6 address with a port (e.g. [fe80:102::2%eth1]:3306).

The MySQL (PDO) and the PostgreSQL (PDO) drivers seem not to do that. The PDO driver base class determines the data source string here: https://github.com/joomla-framework/database/blob/3.x-dev/src/Pdo/PdoDriver.php#L128-L321

I have to dig deeper to see why it works with MySQL (PDO) but not with PostgreSQL (PDO).

@alikon
Copy link
Contributor

alikon commented Aug 15, 2024

a draft pr #43926 about

There is a need to extend administrator | Global configuration | Server to show the port number

@richard67
Copy link
Member

a draft pr #43926 about

There is a need to extend administrator | Global configuration | Server to show the port number

Well that how to do it with a new field in global configuration, which is of course the clean way.

On the other hand, we do not really want to add new fields if we can avoid it.

The alternative to the PR mentioned above would be to add similar code to the other database drivers as we have it in the MySQLi driver in the connect method to extract the port from the host name when it has a port at the end, separated from the rest of the host name by a colon.

I know that might be more ugly than a new configuration field, as all database drivers already support it to get the port with a separate parameter. But it would make all 3 drivers behave in the same way like the MySQLi driver already does.

@muhme
Copy link
Contributor Author

muhme commented Aug 24, 2024

it would make all 3 drivers behave in the same way like the MySQLi driver already does

Note, as it was tested today for the joomla-cypress PR: Currently all four drivers mysqli, mysql, mariadbi and mariadb work in Web Installer with host:port, only pgsql does not

muhme added a commit to muhme/joomla-cypress that referenced this issue Aug 24, 2024
This is one puzzle piece for joomla/joomla-cms#43902
laoneo pushed a commit to joomla-projects/joomla-cypress that referenced this issue Aug 30, 2024
This is one puzzle piece for joomla/joomla-cms#43902
@muhme
Copy link
Contributor Author

muhme commented Aug 31, 2024

@alikon and me had a video call today to align and to see what are the next steps:

  • We are in favour of a solution for non-standard database ports with an additional field in the web installer and in the backend administration screens.
  • We assume that it will be a 5.3 enhancement.
  • Strictly speaking, it is about the non-standard database port field. It is therefore an optional field that is initially empty. The default port is not entered in the form, as otherwise it may have to be changed if the user changes the database type.
  • Additional open TODOs (until the updated list in the initial description) are
    • Impement in driver/postgresql.sql
    • Display in System Information | Configuration File
  • Heiko will test [5] add host database port #43926 together with add db_port joomla-projects/joomla-cypress#34

@HLeithner
Copy link
Member

I'm not a fan of a new field if you get the value as everyone else in a couple of lines of code:
https://3v4l.org/oFFaE or a bit cleaned up https://3v4l.org/C2ADn

<?php

$values = [
    'ip' => '127.0.0.1',
    'hostname' => 'localhost',
    'ipport' => '127.0.0.1:3307',
    'hostname' => 'localhost:3308',
    'ipv6' => '[2001:0DB8::1234]',
    'ipv6port' => '[2001:0DB8::1234]:3309',
    ];
    
foreach($values as $k=>$hostport) {
$host = $hostport;
$port = 'not found';

// has port or ipv6
if (strpos($hostport, ':')) {
    // ipv6
    if (str_starts_with($hostport, '[')) {
        $portposition = strpos($hostport,']:');
        if ($portposition) {
            
          $port = substr($hostport, $portposition+2);
          $host = substr($hostport, 0, $portposition+1);
        }
    } else {
        [$host, $port] = explode(':', $hostport);
    }
}

echo $k .': Hostname: ' . $host . ' Port: ' . $port ."\n";

}

don't add options if we don't really need it, the database stuff as already many options, especially if it comes to encryption

@richard67
Copy link
Member

As far as I remember, the database drivers (framework) should already support to pass the port number as a separate option. The MySQL drivers contain code to extract the port number from the host name if it is given with the host name (appended with colon) and the separate option is not given. The PostgreSQL driver does not contain that code.

See my comment above with links to the relevant parts of the code: #43902 (comment)

I think we should extend the PostgreSQL driver in the framework so that it behaves the same as the other drivers and you can specify the port either appended to the host name or given with the separate option.

In this way we would not need a new field in the installation and the backend (but could add it later if we want), but when you have a 3rd party extension with code to connect to another database, they could use the separate option to tell the driver which port to use.

@HLeithner
Copy link
Member

sorry didn't read you comment only the post by @muhme so yes copy the code from mysql to pgsql or parts of it to the connect function is better in my opinion.

@richard67
Copy link
Member

@HLeithner Your code should not use the same array key 'hostname' for 2 cases. I would suggest to use 'hostnameport' for the 2nd one, so you have:

$values = [
    'ip' => '127.0.0.1',
    'hostname' => 'localhost',
    'ipport' => '127.0.0.1:3307',
    'hostnameport' => 'localhost:3308',
    'ipv6' => '[2001:0DB8::1234]',
    'ipv6port' => '[2001:0DB8::1234]:3309',
    ];

But anyway I think this code (or other code which we already have and which does the same thing) should not be in the CMS, it should be in the database driver.

@richard67
Copy link
Member

@muhme @alikon I have created a PR in the database framework. Please test joomla-framework/database#310 . But @alikon please don't close this issue here until we have a new version with that PR in the framework and a CMS PR to update to that new version.

@richard67
Copy link
Member

P.S.: @alikon 's PR #43926 should still work with the changes from my framework PR.

@muhme
Copy link
Contributor Author

muhme commented Sep 2, 2024

Since @HLeithner and @richard67 have argued against a new "Non-Standard Database Port" field, and @richard67 has already taken action thankfully with joomla-framework/database#310, @alikon and me are okay with implementing and documenting the non-standard database port as host:port without having a new field in the Web Installer and backend forms.

laoneo pushed a commit to joomla-projects/joomla-cypress that referenced this issue Oct 20, 2024
* Use db_port

This is one puzzle piece for joomla/joomla-cms#43902

* Wrap IPv6 in brackets [ ] if port is specified.

If a port number is provided, wrap IPv6 addresses with square brackets [ ].

* No square brackets for IPv6 address for PostgreSQL

* installJoomla for stable releases (#35)

* Update README.md (#32)

* Update README.md

Correcting image file name

* Use absolute URL to see image in NPM README too

* Square brackets on pgsql IPv6 addresses with port
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants