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

J4: MysqlDriver::insertObject vs DatabaseDriver::updateObject #35520

Closed
stAn47 opened this issue Sep 9, 2021 · 5 comments
Closed

J4: MysqlDriver::insertObject vs DatabaseDriver::updateObject #35520

stAn47 opened this issue Sep 9, 2021 · 5 comments

Comments

@stAn47
Copy link

stAn47 commented Sep 9, 2021

Steps to reproduce the issue

reproduce:
insert a row where DateTime column is '0' -> in this case insertion does not fail and a new row is inserted
update a row where DateTime column is '0' -> an error is provided from mysql (Incorrect datetime value: '0' for column... )

Expected result

  1. either both should fail
  2. or none should fail

there seems to be a code inconsitancy in Joomla 4 database drivers as insertObject ignores some of the columns with code:
\libraries\vendor\joomla\database\src\Mysql\MysqlDriver.php

foreach (get_object_vars($object) as $k => $v)
		{
			// Skip columns that don't exist in the table.
			if (!array_key_exists($k, $tableColumns))
			{
				continue;
			}

			// Only process non-null scalars.
			if (\is_array($v) || \is_object($v) || $v === null)
			{
				continue;
			}

			// Ignore any internal fields.
			if ($k[0] === '_')
			{
				continue;
			}

			// Ignore null datetime fields.
			if ($tableColumns[$k] === 'datetime' && empty($v))
			{
				continue;
			}

			// Ignore null integer fields.
			if (stristr($tableColumns[$k], 'int') !== false && $v === '')
			{
				continue;
			}

			// Prepare and sanitize the fields and values for the database query.
			$fields[] = $this->quoteName($k);
			$values[] = $this->quote($v);
		}

while updateObject does not use same logic as above:
\libraries\vendor\joomla\database\src\DatabaseDriver.php

foreach (get_object_vars($object) as $k => $v)
		{
			// Skip columns that don't exist in the table.
			if (!\array_key_exists($k, $tableColumns))
			{
				continue;
			}

			// Only process scalars that are not internal fields.
			if (\is_array($v) || \is_object($v) || $k[0] === '_')
			{
				continue;
			}

			// Set the primary key to the WHERE clause instead of a field to update.
			if (\in_array($k, $key, true))
			{
				$where[] = $this->quoteName($k) . ($v === null ? ' IS NULL' : ' = ' . $this->quote($v));

				continue;
			}

			// Prepare and sanitize the fields and values for the database query.
			if ($v === null)
			{
				// If the value is null and we want to update nulls then set it.
				if ($nulls)
				{
					$val = 'NULL';
				}
				else
				{
					// If the value is null and we do not want to update nulls then ignore this field.
					continue;
				}
			}
			else
			{
				// The field is not null so we prep it for update.
				$val = $this->quote($v);
			}

			// Add the field to be updated.
			$fields[] = $this->quoteName($k) . '=' . $val;
		}

i believe both of them should use same definition on columns which are going to be ignored.

best regards, stan

@richard67
Copy link
Member

That should be reported here https://github.com/joomla-framework/database/issues for the 2.0-dev branch (which is used in Joomla 4).

@alikon
Copy link
Contributor

alikon commented Sep 10, 2021

in this way is a bit generic imo, can you please post an example (code)

@stAn47
Copy link
Author

stAn47 commented Sep 13, 2021

example code - to be run as domain.com/test.php directly

<?php
define('_JEXEC', 1);

if (!defined('_JDEFINES'))
{
	define('JPATH_BASE', dirname(__FILE__));
	require_once JPATH_BASE . '/includes/defines.php';
}
require_once JPATH_BASE . '/includes/framework.php';

// Boot the DI container
$container = \Joomla\CMS\Factory::getContainer();
$container->alias('session.web', 'session.web.site')
	->alias('session', 'session.web.site')
	->alias('JSession', 'session.web.site')
	->alias(\Joomla\CMS\Session\Session::class, 'session.web.site')
	->alias(\Joomla\Session\Session::class, 'session.web.site')
	->alias(\Joomla\Session\SessionInterface::class, 'session.web.site');

// Instantiate the application.
$app = $container->get(\Joomla\CMS\Application\SiteApplication::class);

// Set the application as global app
\Joomla\CMS\Factory::$application = $app;

$q = 'create table if not exists #__test ( `id` INT NOT NULL AUTO_INCREMENT , `mydate` datetime NOT NULL DEFAULT CURRENT_TIMESTAMP, `data` VARCHAR(100) NOT NULL , PRIMARY KEY (`id`)) ENGINE = InnoDB;'; 
$db = JFactory::getDBO(); 
$db->setQuery($q); 
$db->execute(); 

class JTableTest extends JTable {
	var $id = null;
	var $mydate = '0'; 
	var $data = ''; 
	
}

$table = new JTableTest('#__test', 'id', $db); 

/* INSERT */
$datas = array(); 
$datas['id'] = null; 
$datas['mydate'] = '0'; 
$datas['data'] = 'test'; 

$x = $table->bind($datas); 
$x = $table->store(); 


/* UPDATE */
$q = 'select `id` from #__test where `data` = \'test\''; 
$db->setQuery($q); 
$id = $db->loadResult(); 

$datas = array(); 
$datas['id'] = (int)$id; 
$datas['mydate'] = '0'; 
$datas['data'] = 'test'; 

$x = $table->bind($datas); 
$x = $table->store(); 

var_dump($table->getErrors()); 


result:

  1. insert is OK
  2. update fails with - "Incorrect datetime value: '0' for column 'mydate' at row 1"

best regards, stan

@richard67
Copy link
Member

Closing here as it will be handled in the framework repository, see joomla-framework/database#255 .

@stAn47 Thanks for reporting.

@alikon
Copy link
Contributor

alikon commented Sep 13, 2021

thank you @stAn47
i'll follow up on joomla-framework/database#255

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants