Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cast boolean to integer in DC_Table::copy #6473

Merged
merged 4 commits into from Nov 2, 2023

Conversation

fritzmg
Copy link
Contributor

@fritzmg fritzmg commented Oct 30, 2023

Suppose you have the following DCA adjustment within Contao 4.13:

// contao/dca/tl_content.php
use Contao\CoreBundle\DataContainer\PaletteManipulator;

$GLOBALS['TL_DCA']['tl_content']['fields']['foobar'] = [
    'inputType' => 'checkbox',
    'eval' => [
        'doNotCopy' => true,
    ],
    'sql' => [
        'type' => 'boolean',
        'default' => false,
    ],
];

PaletteManipulator::create()
    ->addField('foobar', 'expert_legend', PaletteManipulator::POSITION_APPEND)
    ->applyToPalette('text', 'tl_content')
;

If you then copy a content element, the following error will occur:

PDOException:
SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect integer value: '' for column `c413`.`tl_content`.`foobar` at row 1

  at vendor\doctrine\dbal\src\Driver\PDO\Statement.php:121
  at PDOStatement->execute(null)
     (vendor\doctrine\dbal\src\Driver\PDO\Statement.php:121)
  at Doctrine\DBAL\Driver\PDO\Statement->execute(null)
     (vendor\doctrine\dbal\src\Driver\Middleware\AbstractStatementMiddleware.php:69)
  at Doctrine\DBAL\Driver\Middleware\AbstractStatementMiddleware->execute(null)
     (vendor\doctrine\dbal\src\Logging\Statement.php:98)
  at Doctrine\DBAL\Logging\Statement->execute(null)
     (vendor\doctrine\dbal\src\Driver\Middleware\AbstractStatementMiddleware.php:69)
  at Doctrine\DBAL\Driver\Middleware\AbstractStatementMiddleware->execute(null)
     (vendor\symfony\doctrine-bridge\Middleware\Debug\Statement.php:72)
  at Symfony\Bridge\Doctrine\Middleware\Debug\Statement->execute()
     (vendor\doctrine\dbal\src\Connection.php:1096)
  at Doctrine\DBAL\Connection->executeQuery('INSERT INTO tl_content (array(…), array())
     (vendor\contao\contao\core-bundle\src\Resources\contao\library\Contao\Database\Statement.php:286)
  at Contao\Database\Statement->query('', array('text', false, 1, 'tl_article', 24, 0, '…', '', '', '', 'ascending', 0, '', '', '', '', '', ''))
     (vendor\contao\contao\core-bundle\src\Resources\contao\library\Contao\Database\Statement.php:235)
  at Contao\Database\Statement->execute()
     (vendor\contao\contao\core-bundle\src\Resources\contao\drivers\DC_Table.php:929)
  at Contao\DC_Table->copy()
     (vendor\contao\contao\core-bundle\src\Resources\contao\classes\Backend.php:667)
  at Contao\Backend->getBackendModule('article', null)
     (vendor\contao\contao\core-bundle\src\Resources\contao\controllers\BackendMain.php:168)
  at Contao\BackendMain->run()
     (vendor\contao\contao\core-bundle\src\Controller\BackendController.php:49)
  at Contao\CoreBundle\Controller\BackendController->mainAction()
     (vendor\symfony\http-kernel\HttpKernel.php:163)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
     (vendor\symfony\http-kernel\HttpKernel.php:75)
  at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
     (vendor\symfony\http-kernel\Kernel.php:202)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (public\index.php:44)

This PR would fix this - by backporting a change we introduced in 94c2feb.

Note: checkbox fields with boolean database types are supported since Contao 4.9.11.

@fritzmg fritzmg added the bug label Oct 30, 2023
@fritzmg fritzmg added this to the 4.13 milestone Oct 30, 2023
@fritzmg fritzmg self-assigned this Oct 30, 2023
@fritzmg
Copy link
Contributor Author

fritzmg commented Oct 30, 2023

Note: our StatementTest already assumed that boolean values are cast to integer.

ausi
ausi previously approved these changes Oct 31, 2023
@ausi
Copy link
Member

ausi commented Oct 31, 2023

Did you test if the char(1) fields still work after that change? Or does false for them now result in '0' instead of ''?

@fritzmg
Copy link
Contributor Author

fritzmg commented Oct 31, 2023

DC_Table won't pass a boolean for these fields to Statement::set and thus nothing is changed. But I will double check.

@fritzmg
Copy link
Contributor Author

fritzmg commented Oct 31, 2023

@ausi oh, or do you mean if somebody uses

Contao\Database::getInstance()
    ->prepare("INSERT INTO tl_content %s")
    ->set(['invisible' => false, …])
    ->execute()
;

which previsouly would have, by chance, inserted '' and now would insert '0'?

@ausi
Copy link
Member

ausi commented Oct 31, 2023

Yes, not sure if we would even need to support the custom query with a false parameter case.
But we should check how Model::save() handles this, because setting boolean values on the models is commonly used AFAIK.

@fritzmg fritzmg marked this pull request as draft October 31, 2023 14:43
@fritzmg
Copy link
Contributor Author

fritzmg commented Nov 1, 2023

@ausi you are right, this would then save '0' to the char(1) fields instead of ''. Technically this is not an issue within Contao itself, as Contao always checks for =1/!=1 rather than !=''/=''. Though I think we should not change this behavior in Contao 4.13 (the behavior is changed in Contao 5 already though).

I'll see if we can fix this particular issue in DC_Table instead.

@fritzmg fritzmg marked this pull request as ready for review November 1, 2023 11:56
@fritzmg
Copy link
Contributor Author

fritzmg commented Nov 1, 2023

Another way to fix this would be to change

if (\in_array($sql['type'], array(Types::BIGINT, Types::DECIMAL, Types::INTEGER, Types::SMALLINT, Types::FLOAT)))
{
return 0;
}
if ($sql['type'] === Types::BOOLEAN)
{
return false;
}

to

-	if (\in_array($sql['type'], array(Types::BIGINT, Types::DECIMAL, Types::INTEGER, Types::SMALLINT, Types::FLOAT)))
+	if (\in_array($sql['type'], array(Types::BIGINT, Types::DECIMAL, Types::INTEGER, Types::SMALLINT, Types::FLOAT, Types::BOOLEAN)))
	{
		return 0;
	}
-
-	if ($sql['type'] === Types::BOOLEAN)
-	{
-		return false;
-	}

But this may have unintended consequences so I think we should fix this case by case instead.

The current version of this PR fixes this for the doNotCopy case within DC_Table. This does not need to be merged upstream to Contao 5, as there we cast all booleans always to integers.

@fritzmg fritzmg changed the title Cast boolean to integer in Statement::query Cast boolean to integer in DC_Table::copy Nov 1, 2023
@leofeyer leofeyer enabled auto-merge (squash) November 2, 2023 13:46
@leofeyer
Copy link
Member

leofeyer commented Nov 2, 2023

Thank you @fritzmg.

@leofeyer leofeyer merged commit 47287e0 into contao:4.13 Nov 2, 2023
8 of 16 checks passed
@fritzmg fritzmg deleted the cast-bool-to-int branch November 2, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants