contao / installation-bundle

[READ-ONLY] Contao Installation Bundle
GNU Lesser General Public License v3.0
8 stars 9 forks source link

Can no longer install with Doctrine DBAL 2.5 #54

Closed leofeyer closed 7 years ago

leofeyer commented 7 years ago

Doctrine DBAL 2.5 has added the server_version parameter. It it is not given, Doctrine tries to read it from the MySQL server, thus enabling a database connection, which leads to an Access denied for user ''@'localhost' exception.

Related issue: https://github.com/kriswallsmith/assetic/issues/681

leofeyer commented 7 years ago

@contao/developers @fritzmg /cc

aschempp commented 7 years ago

Doctrine tries to read it from the MySQL server

Who's Doctrine? The bundle? The bundle extension?

Toflar commented 7 years ago

When does that error happen? When you call the install tool? The screenshot does not include the whole stack trace :)

leofeyer commented 7 years ago

The problem is not limited to the install tool. It occurs everywhere.

leofeyer commented 7 years ago

See: https://github.com/doctrine/dbal/issues/990 See: https://github.com/doctrine/DoctrineBundle/issues/351

aschempp commented 7 years ago

The quickest solution would be to add a conflict with 2.5?

aschempp commented 7 years ago

The solution is fairly simple. We cannot assume a complete installation without a working DB connection. Adjust to the following:

// class Contao\Config
public static function isComplete()
{
    if (static::$blnHasLcf === null || !static::has('licenseAccepted'))
    {
        return false;
    }

    try
    {
        \Database::getInstance()->listTables();
    }
    catch (\Exception $e)
    {
        return false;
    }

    return true;
}
fritzmg commented 7 years ago

@aschempp but this would introduce two additional database queries on each request.

aschempp commented 7 years ago

True, we should probably perform the DB check only when we're in the install tool.

Another option would be to store isComplete in the container, but that's rather strange imho.

leofeyer commented 7 years ago

This does not fix the issue, because the problem originates somewhere else.

https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Connection.php#L429

As you can see in the stack trace, the detectDatabasePlatform() method is causing the issue, because it tries to connect to the database before the database credentials have been set.

aschempp commented 7 years ago

It does fix the issue in my installation. detectDatabasePlatform is only called because someone tries to connect, in my case it's the cron job listener checking if table tl_cron exists.

leofeyer commented 7 years ago

Nope, sorry. Your fix is not working.

leofeyer commented 7 years ago

And just so you see that the issue is not at all related to the install tool:

aschempp commented 7 years ago

Well to me the install tool must work without DB, everything else cannot work. Idk why the command tries to connect to the database?

leofeyer commented 7 years ago

Because Doctrine DBAL tries to connect to the database to retrieve the version number.

https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Connection.php#L429

And this already happens in ConnectionFactory::createConnection(), which is executed upon $container->get('doctrine.dbal.default_connection');.

And all of this happens way before the installation controller is even constructed!

aschempp commented 7 years ago

In my case no connection to the database was made if noone queried something…

fritzmg commented 7 years ago

This error will probably only occur, if you do not allow anonymous connections to your database server (see also https://github.com/contao/core-bundle/issues/736).

leofeyer commented 7 years ago

Yes, that's true. If anonymous connections are allowed, the issue does not occur. But that's rarely the case fortunately.

leofeyer commented 7 years ago

I have commented here to document the issue.

leofeyer commented 7 years ago

BTW, this issue also affects the managed edition:

leofeyer commented 7 years ago

It seems that our BinaryStringType is the reason why the getDatabasePlatform() method is called in the ConnectionFactory class. We might be able to register it in an compiler path to work around it.

mattjanssen commented 7 years ago

Sounds like your GUI config bundle needs to be able to handle when the DB credentials are missing or are no longer valid? Perhaps you could register a kernel.exception event listener that disables the doctrine config section once a DB ConnectionException is thrown. It could do this by either editing config.yml or switching to the "install" environment which would load config_install.yml. Finally it redirects the user with a RedirectResponse to the GUI admin login page with a nice error.

This is assuming your GUI config bundle is in the business of editing config.yml, which seems like a possibility :)

leofeyer commented 7 years ago

@mattjanssen Thanks a lot for your input. Much appreciated.

leofeyer commented 7 years ago

We are now dynamically adding server_version: 5.1 in the Contao managed edition, if there is no database connection: https://github.com/contao/manager-bundle/commit/06f1f294bedcbe456869145597de0dfa32f352aa

aschempp commented 7 years ago

@leofeyer should we add an is_array check in https://github.com/contao/manager-bundle/commit/fd465bce6d55491c652dfad49a695485b6dd69b1#diff-e3375107bffc209723fa127bb186d8c6R146 ?

leofeyer commented 7 years ago

If $extensionConfig['dbal']['connections']['default'] is set, it should always be an array, shouldn't it?

Toflar commented 7 years ago

Unit tests would tell you :P

leofeyer commented 7 years ago

No true in this case. If $extensionConfig['dbal']['connections']['default'] is not an array, the user has misconfigured the application.

doctrine:
    dbal:
        connections:
            default: 'string'  <--  this is wrong!
aschempp commented 7 years ago

Unit tests would tell you :P

Not really, because the data is coming from other bundles. I think the unifying of TreeBuilder is only done after our method. According to https://github.com/doctrine/DoctrineBundle/blob/master/DependencyInjection/Configuration.php#L69 it's actually possible to set the connection details directly in doctrine.dbal but I think that's just an unsupported case for us then, especially because we know that the config loaded by manager-bundle will not be like that.

fritzmg commented 7 years ago

Is this actually fixed in Contao 4.4.0? I am unable to open the Install Tool once I have removed the anonymous user accounts from my SQL database. Stack trace seems to be the same as posted by @leofeyer

// nvm, suddenly it worked after clearing the dev cache