contao / installation-bundle

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

Installation tool does not correctly encode cols named 'row' #92

Closed ghost closed 6 years ago

ghost commented 6 years ago

Since newer versions of MariaDB and MySQL, ROWS is a reserved keyword.

Unfortunately, some DCA fields in Contao (e.g. tl_layout.rows) are named rows. The install tool does not quote this correctly as `rows`:

ALTER TABLE tl_layout CHANGE rows rows VARCHAR(8) DEFAULT '' NOT NULL

This only affects the install tool, internal usage for queries and update statements are escaped correctly.

Issue opened upon request by @Toflar

ausi commented 6 years ago

Probably related: https://github.com/contao/installation-bundle/pull/70#issuecomment-353210582

Which versions of Contao, PHP and MariaDB are you using?

ghost commented 6 years ago
ghost commented 6 years ago

I am going to provide @Toflar with a demo account for the environment anyway, because of another bug we talked about at the conference. If it helps, the account could also be used for this.

m-vo commented 6 years ago

IIRC MariaDB 10.2 needs Doctrine 2.7 which isn't released yet.

(see #83)

ausi commented 6 years ago

@sleitz Your combination of the Contao, PHP and MariaDB versions should work AFAIK. Can you please also check which version of doctrine/dbal is installed? (Can be done with the command composer show doctrine/*)

IIRC MariaDB 10.2 needs Doctrine 2.7 which isn't released yet.

@m-vo the issue with the rows keyword should already be solved in Doctrine 2.6. Also 2.7 was released in April.

ghost commented 6 years ago

That's a good hint:

doctrine/dbal                  v2.5.13 
ausi commented 6 years ago

That’s the issue. You should update at least to doctrine/dbal 2.6.0 to fix the problem.

ghost commented 6 years ago

Well, it was not a voluntary choice. That's the version I get when I install the managed edition.

fritzmg commented 6 years ago

The composer.lock file of the contao/managed-edition is built with the lowest PHP version that is still supported - otherwise you might not be able to run the application after installation. In case of Contao 4.4.18 that's PHP 5.6. doctrine/dbal in version 2.6.0 or higher requires at least PHP 7.1. That's why the composer.lock file of Contao 4.4.18 still contains doctrine/dbal in version 2.5.13: https://github.com/contao/managed-edition/blob/4.4.18/composer.lock#L1896

leofeyer commented 6 years ago

We should build the managed edition against PHP 7.1 now that the Contao Manager uses its own composer.json template, shouldn't we?

fritzmg commented 6 years ago

Hm, well what about installation via zip or composer create-project? Also won't this error occur on PHP 5.6 in any case?

ausi commented 6 years ago

Also won't this error occur on PHP 5.6 in any case?

Yes, but we agreed we don’t want to support PHP 5 with Contao 4.4 and MariaDB 10.2.4, see https://github.com/contao/installation-bundle/pull/70#issuecomment-353210582

fritzmg commented 6 years ago

Right, forgot it depends on the MariaDB/MySQL version. @sleitz you simply need to run a composer update in your case, to get the latest dependencies.

leofeyer commented 6 years ago

Hm, well what about installation via zip or composer create-project?

We could build two zip files, one for PHP 5 and one for PHP 7. But since PHP 5.6 is outdated and only supported until the end of 2018, I don't know if it is worth the effort?

Toflar commented 6 years ago

The zip file is useless anyway. Composer or the Manager should be the only 2 ways allowed to install.

ghost commented 6 years ago

@fritzmg I was using the wrong PHP version in CLI, so the new version did not show up (it requires PHP 7.1 itself), and I was using the 7.0 CLI.

My problem is solved, I guess. Don't know about your other discussion, though.

ghost commented 6 years ago

@Toflar This also solved my other issue… :+1:

leofeyer commented 6 years ago

The zip file is useless anyway. Composer or the Manager should be the only 2 ways allowed to install.

I agree, however, a composer create-project will run into the same issue.

Toflar commented 6 years ago

If we do not provide a composer.lock with the managed-edition, everything will work as expected.

leofeyer commented 6 years ago

As discussed in Mumble on July 5th, we want to remove the composer.lock file.

xchs commented 6 years ago

Isn't that any longer true what @naderman said on the conference?

https://www.youtube.com/watch?v=9rr84XofWsA&t=1843

ausi commented 6 years ago

I think this relates to actual projects, not libraries or editions.