contao / installation-bundle

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

DB update loads schema of none Contao tables #78

Closed backbone87 closed 6 years ago

backbone87 commented 6 years ago

https://github.com/contao/installation-bundle/blob/2762a4e103c9c84244c24c3fbb97499c0032e102/src/Database/Installer.php#L98

Here the full database schema is loaded first and non-Contao parts are dropped after. This causes errors, when database specific column types (like MySQL's POINT) are used in non-Contao database assets, that Doctrine doesnt know about:

[2017-12-19 03:09:26] app.CRITICAL: An exception occurred. {"exception":"[object] (Doctrine\\DBAL\\DBALException(code: 0): Unknown database type point requested, Doctrine\\DBAL\\Platforms\\MySqlPlatform may not support it. at /vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php:429)"} []

Doctrine's Schema asset filter option should be used, to prevent loading irrelevant Schema parts:

$config = $this->connection->getConfiguration();
$previousSchemaFilter = $config->getFilterSchemaAssetsExpression();
$config->setFilterSchemaAssetsExpression('@^tl_@');
$fromSchema = $this->connection->getSchemaManager()->createSchema();
$config->setFilterSchemaAssetsExpression($previousSchemaFilter);

Though it would be probably better to use a separate connection configured via doctrine bundle config for the install tool, insteadof "hot"-changing doctrine connection configuration.

leofeyer commented 6 years ago

@contao/developers Any objections?

aschempp commented 6 years ago

I honestly don't think the problem is in Contao, but in the fact that MySQL specific attributes are used that are not supported by Doctrine. @backbone87 you might want to exclude these tables in the application configuration (via schema filters), which should still work in the install tool

leofeyer commented 6 years ago

Why are we not using the schema filter ourselves?

aschempp commented 6 years ago

because it's an application-wide setting that a bundle cannot configure 😂

leofeyer commented 6 years ago

Managed edition == application, isn't it?

leofeyer commented 6 years ago

As discussed in Mumble on December 21st, we want to overwrite the schema filter in the install tool.

leofeyer commented 6 years ago

Fixed in bc4067ab857c2ed2391367b6e14a2cbbdad0b2e9 and contao/core-bundle@4967d3a7adc85d0d706d58e9bdf4726563d08983.

m-vo commented 6 years ago

This change leads to the following side effect:

The $toSchema now also contains regular Doctrine Entities (no tl_ prefix) that the install offers to create as these aren't filtered anymore. However these entries are filtered from the $fromSchema which leads to them still beeing in the list (and of course crashing on update because the table already exists).

Imho the installation tool should either handle all entities or we need to implement a solution like #79 and ignore others. So ironically in this use case it would be better to have no filter set at all. :-)

leofeyer commented 6 years ago

I cannot reproduce this. The schema filter is applied to the $toSchema as well:

https://github.com/contao/core-bundle/blob/e4f0d29c314d22ca2ac235ff938027f48aaf985b/src/Doctrine/Schema/DcaSchemaProvider.php#L356-L363

m-vo commented 6 years ago

This piece of code only filters the entries from $installer->getFromDca(); which do not include the entities. Later those filtered definitions get appended to the schema that contains the doctrine entities.

m-vo commented 6 years ago

To reproduce create a simple entity in a bundle and enable the mapping.

use Doctrine\ORM\Mapping as ORM;

class SomeEntity
{
    /**
     * @ORM\Column(name="id", type="integer")
     * @ORM\Id
     *
     * @var integer $id
     */
    protected $id;
}
doctrine:
    orm:
        entity_managers:
            default:
                mappings:
                    SomeBundle: ~

Clear cache or open the install tool in app_dev.

leofeyer commented 6 years ago

Fixed in contao/core-bundle@d5dcabda26cd9610d7c6717df2946745918c4921. Needs to be backported to Contao 4.4, too.

leofeyer commented 6 years ago

Backported in contao/core-bundle@8109786f5d791b769896e155b3673cec5f327cf1.