Change boolean columns from char(1) to tinyint(1) #4797

fritzmg · 2022-06-06T08:24:21Z

Implements #4184

Note: the migration automatically converts the empty string value of all char(1) fields to 0, if the DCA SQL definition is of type boolean. So this would also automatically be done for any extensions, if applicable.

core-bundle/src/Controller/FrontendModule/TwoFactorController.php

fritzmg · 2022-06-06T09:51:04Z

PHP stan complains about

Property Contao\User::$useTwoFactor (bool|string) does not accept int.

How should we handle this?

Change doc to string|integer and always use 0/1 in assignments.
Change all assignments from 0/1 to false/true.
Change doc to string|integer|boolean.
Change doc to integer|boolean.

Thoughts:

When coming from the database, the value will either be a string or an integer under PDO, depending on the PHP version. However, since Contao 5 already requires PHP 8.1, we can assume that these values are always loaded as integers from the database, correct?
When using the model, users will likely use false/true (in the future) when assigning these variables.

core-bundle/src/Migration/Version500/BooleanFieldsMigration.php

ausi · 2022-06-06T09:57:24Z

~~Change doc to string|integer and always uses 0/1 in assignments.~~

~~I’d favor this one for now I think.~~

Changed my mind to:

Change doc to string|integer|boolean.

ausi · 2022-06-06T10:00:26Z

we can assume that these values are always loaded as integers from the database, correct?

Probably not because of mysqli :(

fritzmg · 2022-06-06T10:04:01Z

Change doc to string|integer and always uses 0/1 in assignments.

I’d favor this one for now I think.

It's not very intuitive though to have to write $model->someBoolean = 1 instead of true.

fritzmg · 2022-06-06T11:07:13Z

I think we would need to go with option no. 3 then in that case? i.e. change the doc to string|integer|boolean because:

when loading from the database the value can either be an integer (PDO) or a string (MySQLi)
when setting values, users will likely want to use true/false

ausi · 2022-06-07T08:26:20Z

I think we should merge this one with string|integer|boolean as this can be changed to boolean in #4800 then.

ausi

❤️

core-bundle/src/Migration/Version500/BooleanFieldsMigration.php

core-bundle/tests/EventListener/DataContainer/PreviewLinkListenerTest.php

core-bundle/tests/Routing/ResponseContext/CoreResponseContextFactoryTest.php

leofeyer · 2022-06-09T14:08:15Z

As discussed, we want to wait for #4800 before we merge this PR.

fritzmg · 2022-06-13T11:21:23Z

Unfortunately I have hit a roadblock here. Doctrine seems to convert parameter values that are false to an empty string, when using

$objDatabase->prepare("INSERT INTO tl_search %s")
            ->set($arrSet)
            ->execute();

With this PR the following error will occur when trying to log into the back end for example:

Doctrine\DBAL\Exception\DriverException:
An exception occurred while executing a query: SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect integer value: '' for column `c50`.`tl_user`.`disable` at row 1

  at vendor\doctrine\dbal\src\Driver\API\MySQL\ExceptionConverter.php:119
  at Doctrine\DBAL\Driver\API\MySQL\ExceptionConverter->convert(object(Exception), object(Query))
     (vendor\doctrine\dbal\src\Connection.php:1814)
  at Doctrine\DBAL\Connection->handleDriverException(object(Exception), object(Query))
     (vendor\doctrine\dbal\src\Connection.php:1749)
  at Doctrine\DBAL\Connection->convertExceptionDuringQuery(object(Exception), '…', array(…), array())
     (vendor\doctrine\dbal\src\Connection.php:1055)
  at Doctrine\DBAL\Connection->executeQuery('…', array(…), array())
     (vendor\contao\contao\core-bundle\src\Resources\contao\library\Contao\Database\Statement.php:269)
  at Contao\Database\Statement->query('', array(…))
     (vendor\contao\contao\core-bundle\src\Resources\contao\library\Contao\Database\Statement.php:228)
  at Contao\Database\Statement->execute(1)
     (vendor\contao\contao\core-bundle\src\Resources\contao\library\Contao\User.php:297)

The functional tests will also fail here for the same reason.

fritzmg · 2022-06-13T12:46:29Z

As briefly discussed with @ausi I am now casting any booleans to integer in Statement::query, so that Doctrine does not convert false to an empty string. However, if you use the Doctrine database connection directly, you will need to make of that sure yourself.

core-bundle/src/Command/UserPasswordCommand.php

fritzmg · 2022-06-29T13:22:45Z

~~There is still an issue with the checkbox widget. It currently gets checked even though the value is 0.~~ // false alarm

core-bundle/src/Resources/contao/dca/tl_page.php

leofeyer

There are still plenty of wrong values in the code base, I‘m afraid. A first quick search revealed the following:

CalendarEventsModel.php line 269
Dbafs.php line 516
FormFieldModel.php line 201
FormModel.php line 121
ImageSizeItemModel.php line 95
MemberGroupModel.php line 83 and 101
MemberModel.php line 197
Module.php line 476
NewsModel.php line 238

We have to make sure to search for ='1' and ='' as well as = '1' and = '' to find all occurrences.

fritzmg · 2022-06-29T14:30:47Z

We have to make sure to search for ='1' and ='' as well as = '1' and = '' to find all occurrences.

Also => '' and => '1'.

I made another sweep.

rabauss · 2022-06-29T14:31:17Z

core-bundle/src/Migration/Version500/BooleanFieldsMigration.php

+            }
+        }
+
+        return $targets;


Are fields of extensions included too, right?

Yes, see my initial comment. However in the new version, any fields of dynamcially added DCAs (like Catalog Manager or MetaModels) would not be included.

leofeyer · 2022-06-29T14:52:06Z

Thank you @fritzmg.

Description ----------- Contao 4 already has support for `boolean` SQL fields. However, there is still one issue remaining: if you try to copy records, you might encounter the following error: ``` Doctrine\DBAL\Exception\DriverException: An exception occurred while executing a query: SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect integer value: '' for column `tl_content`.`foobar` at row 1 ``` This can be reproduced by the following DCA: ```php // contao/dca/tl_content.php $GLOBALS['TL_DCA']['tl_content']['fields']['foobar'] = [ 'eval' => ['doNotCopy' => true], 'sql' => ['type' => 'boolean', 'default' => false], ]; ``` Update the database and then try to copy a page article that contains content elements. This happens because the PDO driver automatically casts everything strings, if no type is given - which for `false` means an empty string - which in turn is not compatible with a `boolean` (`tinyint(1)`) field. To fix this we need to backport [this](https://github.com/contao/contao/blob/110d32fd52f5abfb58a07d15813d09dda7292e8e/core-bundle/contao/library/Contao/Database/Statement.php#L243-L248) from #4797. Commits ------- f2b4709 cast boolean to integer 7753a81 set type instead 4d8e742 cs fix 0126e2c Update core-bundle/src/Resources/contao/library/Contao/Database/State… c67a11e use string param and only fill array if empty d0801f9 fix check 5d992d2 cs fix 8698b23 Only set type to boolean if it wasn’t set explicitly (#8) 70d240f use foreach Co-authored-by: ausi <martin@auswoeger.com>

Description ----------- Contao 4 already has support for `boolean` SQL fields. However, there is still one issue remaining: if you try to copy records, you might encounter the following error: ``` Doctrine\DBAL\Exception\DriverException: An exception occurred while executing a query: SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect integer value: '' for column `tl_content`.`foobar` at row 1 ``` This can be reproduced by the following DCA: ```php // contao/dca/tl_content.php $GLOBALS['TL_DCA']['tl_content']['fields']['foobar'] = [ 'eval' => ['doNotCopy' => true], 'sql' => ['type' => 'boolean', 'default' => false], ]; ``` Update the database and then try to copy a page article that contains content elements. This happens because the PDO driver automatically casts everything strings, if no type is given - which for `false` means an empty string - which in turn is not compatible with a `boolean` (`tinyint(1)`) field. To fix this we need to backport [this](https://github.com/contao/contao/blob/110d32fd52f5abfb58a07d15813d09dda7292e8e/core-bundle/contao/library/Contao/Database/Statement.php#L243-L248) from contao/contao#4797. Commits ------- f2b47094 cast boolean to integer 7753a816 set type instead 4d8e7429 cs fix 0126e2c3 Update core-bundle/src/Resources/contao/library/Contao/Database/State… c67a11e4 use string param and only fill array if empty d0801f9a fix check 5d992d21 cs fix 8698b23b Only set type to boolean if it wasn’t set explicitly (#8) 70d240f7 use foreach Co-authored-by: ausi <martin@auswoeger.com>

fritzmg added feature BC break labels Jun 6, 2022

fritzmg added this to the 5.0 milestone Jun 6, 2022

fritzmg requested review from ausi and leofeyer June 6, 2022 08:24

fritzmg self-assigned this Jun 6, 2022

ausi reviewed Jun 6, 2022

View reviewed changes

core-bundle/src/Controller/FrontendModule/TwoFactorController.php Show resolved Hide resolved

ausi reviewed Jun 6, 2022

View reviewed changes

core-bundle/src/Migration/Version500/BooleanFieldsMigration.php Outdated Show resolved Hide resolved

ausi mentioned this pull request Jun 6, 2022

Cast model values to the correct PHP type #4800

Merged

3 tasks

ausi approved these changes Jun 7, 2022

View reviewed changes

ausi requested changes Jun 7, 2022

View reviewed changes

core-bundle/src/Migration/Version500/BooleanFieldsMigration.php Outdated Show resolved Hide resolved

leofeyer reviewed Jun 8, 2022

View reviewed changes

core-bundle/tests/EventListener/DataContainer/PreviewLinkListenerTest.php Outdated Show resolved Hide resolved

leofeyer reviewed Jun 8, 2022

View reviewed changes

core-bundle/tests/Routing/ResponseContext/CoreResponseContextFactoryTest.php Outdated Show resolved Hide resolved

leofeyer linked an issue Jun 9, 2022 that may be closed by this pull request

Use boolean fields for checkboxes #4184

Closed

fritzmg force-pushed the convert-char1-to-boolean branch from 700143e to 00bb683 Compare June 13, 2022 11:16

fritzmg marked this pull request as draft June 13, 2022 12:31

fritzmg requested review from ausi and leofeyer June 13, 2022 12:47

fritzmg marked this pull request as ready for review June 13, 2022 12:47

fritzmg marked this pull request as draft June 13, 2022 12:52

ausi reviewed Jun 13, 2022

View reviewed changes

core-bundle/src/Command/UserPasswordCommand.php Outdated Show resolved Hide resolved

CS

3bae38a

leofeyer dismissed ausi’s stale review via 3bae38a June 29, 2022 12:53

fritzmg marked this pull request as draft June 29, 2022 13:06

fritzmg added 3 commits June 29, 2022 14:17

fix tl_page.useSsl widget

bfe3b49

fix more empty string checks for booleans

39c98cb

fix tl_page.useSsl widget differently

d114685

fritzmg marked this pull request as ready for review June 29, 2022 13:32

fritzmg requested a review from leofeyer June 29, 2022 13:33

m-vo previously approved these changes Jun 29, 2022

View reviewed changes

leofeyer reviewed Jun 29, 2022

View reviewed changes

core-bundle/src/Resources/contao/dca/tl_page.php Show resolved Hide resolved

leofeyer requested changes Jun 29, 2022

View reviewed changes

another boolean sweep

174bffe

fritzmg dismissed m-vo’s stale review via 174bffe June 29, 2022 14:30

rabauss reviewed Jun 29, 2022

View reviewed changes

fritzmg requested a review from leofeyer June 29, 2022 14:35

Fix two leftover values

5e1b331

leofeyer approved these changes Jun 29, 2022

View reviewed changes

leofeyer merged commit 94c2feb into contao:5.x Jun 29, 2022

fritzmg mentioned this pull request Jun 30, 2022

Fix boolean fields always being saved as true #4916

Merged

leofeyer changed the title ~~Change boolean from char(1) to tinyint(1)~~ Change boolean columns from char(1) to tinyint(1) Jul 15, 2022

This was referenced Dec 18, 2022

Empty accesskeys in navigation #5586

Closed

Fix the tl_page.accesskey database type #5590

Merged

fritzmg mentioned this pull request Jul 15, 2023

Backend favorites don't work #6213

Closed

fritzmg mentioned this pull request Apr 17, 2024

Automatically set ParameterType::BOOLEAN for booleans #7126

Merged

grummbeer mentioned this pull request Sep 23, 2024

Convert the checkboxes to Types::BOOLEAN to help keep the size of the database “row size” small hofff/contao-navigation#35

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change boolean columns from char(1) to tinyint(1) #4797

Change boolean columns from char(1) to tinyint(1) #4797

fritzmg commented Jun 6, 2022

fritzmg commented Jun 6, 2022 •

edited

Loading

ausi commented Jun 6, 2022 •

edited

Loading

ausi commented Jun 6, 2022

fritzmg commented Jun 6, 2022

fritzmg commented Jun 6, 2022

ausi commented Jun 7, 2022

ausi left a comment

leofeyer commented Jun 9, 2022

fritzmg commented Jun 13, 2022 •

edited

Loading

fritzmg commented Jun 13, 2022 •

edited

Loading

fritzmg commented Jun 29, 2022 •

edited

Loading

leofeyer left a comment

fritzmg commented Jun 29, 2022

rabauss Jun 29, 2022

fritzmg Jun 29, 2022

leofeyer commented Jun 29, 2022

Change boolean columns from char(1) to tinyint(1) #4797

Change boolean columns from char(1) to tinyint(1) #4797

Conversation

fritzmg commented Jun 6, 2022

fritzmg commented Jun 6, 2022 • edited Loading

ausi commented Jun 6, 2022 • edited Loading

ausi commented Jun 6, 2022

fritzmg commented Jun 6, 2022

fritzmg commented Jun 6, 2022

ausi commented Jun 7, 2022

ausi left a comment

Choose a reason for hiding this comment

leofeyer commented Jun 9, 2022

fritzmg commented Jun 13, 2022 • edited Loading

fritzmg commented Jun 13, 2022 • edited Loading

fritzmg commented Jun 29, 2022 • edited Loading

leofeyer left a comment

Choose a reason for hiding this comment

fritzmg commented Jun 29, 2022

rabauss Jun 29, 2022

Choose a reason for hiding this comment

fritzmg Jun 29, 2022

Choose a reason for hiding this comment

leofeyer commented Jun 29, 2022

fritzmg commented Jun 6, 2022 •

edited

Loading

ausi commented Jun 6, 2022 •

edited

Loading

fritzmg commented Jun 13, 2022 •

edited

Loading

fritzmg commented Jun 13, 2022 •

edited

Loading

fritzmg commented Jun 29, 2022 •

edited

Loading