Remove wrong '_setup' replace when getting DB connection (2)

[jalogut]

Description

I a second custom database into etc/env.php is added like that, it is not possible to get the database connection using custom_setup as $resourceName:

array (
    'table_prefix' => '',
    'connection' => 
    array (
      'custom' => 
      array (
        'host' => 'localhost',
        'dbname' => '<db_custom>',
        'username' => '<user>',
        'password' => '<pass>',
        'engine' => 'innodb',
        'initStatements' => 'SET NAMES utf8;',
        'active' => '1',
      ),
      'default' => 
      array (
        'host' => 'localhost',
        'dbname' => '<db_default>',
        'username' => '<user>',
        'password' => '<pass>',
        'model' => 'mysql4',
        'engine' => 'innodb',
        'initStatements' => 'SET NAMES utf8;',
        'active' => '1',
      ),
    ),
  ),
  'resource' => 
  array (
    'custom_setup' => 
    array (
      'connection' => 'custom',
    ),
    'default_setup' => 
    array (
      'connection' => 'default',
    ),
  ),

After that I would expect to use the custom database by resource name custom_setup. However, as _setup is always replaced, the system does not find any resource with this name. Then, I need to use a different name without _setup. From my point of view, there is no need to replace that. In fact, the only reason why default_setup database works is because default connection is the fall back. When calling resource default_setup is also not found but it falls back to default.

What is exactly the purpose of replacing _setup? I checked the code and I do not see any need to have that.

Fixed Issue

If a custom database resource contains '_setup', default is returned because $resourceName does not match after replacement

[fooman]

What is exactly the purpose of replacing _setup? I checked the code and I do not see any need to have that.

To me this looks like it would rely on the functionality that is removed. As to why this is implemented in such a way I don't know.

[jalogut]

Hi @fooman ,

Actually the code that you mention has nothing to do with that. This 2 lines of code are simply creating a new instance of Magento\Quote\Setup\QuoteSetup and Magento\Sales\Setup\SalesSetup. The resourceName parameter is not even used as there is not such argument on the __construct of these classes. See this and this

I have checked all calls to the method edited in this pull request and the only $resourceName passed to this function containing the word _setup is default_setup. Even in this case the logic will not find a connectionName for resourceName = default, so the default connection \Magento\Framework\App\ResourceConnection::DEFAULT_CONNECTION is always used.

For me there is no such need to replace _setup. This only prevents a custom database to use _setup in the resourceName. That is a bit weird, if we want to be consistent with Magento and use custom_setup same Magento uses default_setup

[fooman]

Thanks @jalogut for the explanation. I'll progress this PR along (this will run tests on the EE version as well) and will try to get someone from the Magento side to let us know why this was done in the first place.

[fooman]

@jalogut apologies for the delay. The internal testsuite looks all good for your proposed changes. Would you mind quickly addressing this mini style issue highlighted by codacy here https://www.codacy.com/app/antonkril/magento2/pullRequest?prid=592044

[jalogut]

Hi @fooman,

Done.

[fooman]

@jalogut good stuff - will keep you posted.

[fooman]

Thank you @jalogut for your contribution - it's been merged.