Add database port to Magento Setup Model Installer

[tkn98]

Take optional database port setting into account when connecting to the database with the Magento Setup Model.

Previously it was not possible to use magento setup with a non-standard database port.

[buskamuza]

Hi @tkn98 ,

thanks for the contribution. But you can specify port as part of the host option. See http://devdocs.magento.com/guides/v2.0/install-gde/install/cli/install-cli-install.html

[tkn98]

@buskamuza: Thanks for the insights you present about the command-line switch.

In my scenario, I was referring to app/etc/env.php, not a command-line parameter. In any case as I wrote, there is a problem with the way how the setup class obtains the database configuration and the way it's done within the rest of the application.

So this report / PR is probably a good point to pin-point the exact cause.

First of all, I can confirm that it's possible to use the port number within the host configuration setting in app/etc/env.php as well.

But if the port configuration value is not to be used, then it still needs blacklisting. If it should be used, then it needs the support in the setup class as suggested.

With this new information, would you argue with the information given from your comment, that the port setting in env.php most likely should be blacklisted because I was using and it does work, too, but it was wrong to use in any case? Or are the consequences you point out are in the different direction that the setup class should whitelist the (then optional) port setting?

/Edit: PHP PDO DSN named elements documentation is here: http://php.net/manual/en/ref.pdo-mysql.connection.php

[buskamuza]

Hi @tkn98 ,

could you please clarify what do you mean under "blacklisting"?

General idea in Magento is that port is expected in the "host" configuration. See https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/DB/Adapter/Pdo/Mysql.php#L327
You mentioned that it works for you. If in some case it doesn't work, could you, please provide more details?

[tkn98]

Hi @buskamuza,

sure, I'll try to make more clear what I mean by blacklisting and my explanation should also show why in my scenario it doesn't work.

The $this->_config array-accessed property you reference in line 327 ff is later on with certain named elements "blacklisted" in Zend_Db_Adapter_Pdo_Abstract::_dsn():

    // don't pass the username, password, charset, persistent and driver_options in the DSN
    unset($dsn['username']);
    unset($dsn['password']);
    unset($dsn['options']);
    unset($dsn['charset']);
    unset($dsn['persistent']);
    unset($dsn['driver_options']);

What is named $dsn here is actually $this->_config (Zend_Db_Adapter_Pdo_Abstract line 63):

    // baseline of DSN parts
    $dsn = $this->_config;

That means that the config setting $this->_config['port']which you outlined to be the product of the explode() operation is actually also injectable via env.php as array entry (named 'port').

You argue that the code that containing the explode() is showing that the port is to be passed via <hostname[:port]>, however, the port entry is not unset first so that the exact portion of the code you reference is additionally an equally good example for allowing to pass the port in via the port config element as well (both is shown possible by that piece of code).

A concrete example of blacklisting the port entry then would be to unset the port element first before occasionally setting it (first line ist added to your example):

    unset($this->_config['port']); # port is configured via the "host" element <hostname:port> _only_

    if (strpos($this->_config['host'], '/') !== false) {
        $this->_config['unix_socket'] = $this->_config['host'];
        unset($this->_config['host']);
    } elseif (strpos($this->_config['host'], ':') !== false) {
        list($this->_config['host'], $this->_config['port']) = explode(':', $this->_config['host']);
    }

This little change would effectively prevent that the env.php configuration has different interpretations on the port setting when used in store or in setup. This change is what I meant by saying "blacklisting" as removing the port entry in case it was added in env.php not via the host element.

I hope this gives a better example and also outlines better the data-flow that I experienced in my scenario which led to the different interpretation of the configuration values.

[tkn98]

For reference the beginning of the env.php that lead to the problem, see the port entry:

<?php
return array(
    'backend'         =>
        array(
            'frontName' => 'admin',
        ),
    'db'              =>
        array(
            'connection'   =>
                array(
                    'default' =>
                        array(
                            'host'           => '127.0.0.1',
                            'port'           => '33390', # port entry in error
                            'dbname'         => 'zeee_shop',
                            'username'       => 'root',
                            'password'       => 'root',
                        ),
                ),
            'table_prefix' => '',
        ),
[...]

[buskamuza]

@tkn98 , thanks for the explanation.
It's a very good point. I've created an internal ticket to review it (MAGETWO-47110).
I agree that there should be only one way to specify "port" to not confuse. But I'll discuss it with product owner(s) to decide on a final approach.

[buskamuza]

Hi once again, @tkn98 .
Internally we agreed to prohibiting usage of "port" as a separate configuration field. An exception should be thrown in this case with suggestion to put port in the "host" field.
Does it make sense for you?

Also, if you would agree to prepare a new PR with such behavior, we'll be happy to accept it and it should be faster than processing it internally.

[tkn98]

@buskamuza: Hi Olga, I'm very much for blocking the "port" configuration field as I think that not-blocking (not blacklisting) it is the origin of the flaw. Also I'm very much for introducing simplicity by having one way to configure things - consistence should be aimed for as much as possible.

About the exception: Apart from taste, the existing code in Zend_DB does not throw exceptions on named configuration fields it drops, it just unsets them without notice (ref.). Just saying, that place in code is not entirely comparable (but related) with the port issue we discuss here.

I'm personally fine with throwing exceptions, so that the configuration issue won't get unnoticed then for a user who didn't see the issue so far but has it "configured". If the message gives a good hint where to configure the port, everybody should be happy.

[buskamuza]

Thanks, @tkn98 .
Do you think #2736 is still actual?

[tkn98]

@buskamuza: If the "port" entry is forbidden then #2736 is obsolete as it is not missing any longer (as it is part of the "host" entry and it can't be used at all any longer).

[buskamuza]

Thanks, @tkn98

[tkn98]

@buskamuza: No problem, just ping me if you need anything else from my end. And thank you for your helpful and constructive assistance with the issue.

[ktomk]

@buskamuza (from a private account here) please let me know if you'd like to have a pull request for this. I can do one. Currently I'm progressing the CLA within the company. Happy new year btw..

[buskamuza]

@ktomk , sure, a pull request would be a great help.
Thanks. Happy New Year! :)

[vancoz]

Hello @tkn98, please merge latest changes from develop and rerun builds.

[ktomk]

@vancoz: As decided this needs a different patch, too. I'll replace it at times ETA sometime on Thursday.

[vancoz]

Thank you @ktomk!

[tkn98]

@vancoz: Please find this PR updated with the patch.

[tkn98]

@vancoz: Please trigger the build again, it should have been green but one of the machines went down.

[davidalger]

@tkn98 I've restarted the failed job in the build.

[tkn98]

@mazhalai: I see some traction on other tickets, I think this one is a probable candidate, too, isn't it (the nees update label I think is stale /cc @vancoz) ?

[KrystynaKabannyk]

Hello @tkn98, this PR has been merged in the 2.1.0 Release, that's why I'm closing it. If you any questions or additional information regarding the PR feel free to reopen it or create a new one.

[shiftedreality]

Looks like this was not merged.
Reopened for processing.

[tkn98]

Hey @KrystynaKabannyk, can we please share the hash of the commit where this has been fixed?