The issue occurs when you have more than one record to bind and save.
eg.
foreach($recordset as $record)
{
$table->save($record);
}
If the record contains fields that admit null and that are set to null, they don't get bound properly by the bind() function.
The first record to be saved works properly, because the Table object is freshly instantiated and initialized.
During subsequent bind() operations, fields containing null values don't overwrite table properties already set to non-null.
I created a new PHPunit method in \tests\Integration\Libraries\Cms\Table\TableTest.php, to better explain the issue
/**
* Test that bind() properly bind a null field
*
* @return void
* @since 4.1.0
*/ public function testBindNullFieldsWithoutReset()
{
$data_1 = [
'title' => 'Title One',
'checked_out' => 1
];
$data_2 = [
'title' => 'Title Two',
'checked_out' => null
];
// Check if nullable int property is 1
$this->object->bind($data_1);
$this->assertEquals(1, $this->object->checked_out);
// Check if nullable int property is null
$this->object->bind($data_2);
$this->assertEquals(null, $this->object->checked_out);
}
When I run the test above, I expect that bind($data_2) sets 'checkout' property to null.
The actual result is that the 'checkout' property remains set to 1.
PHP Version 8.0.19
Web Server Apache/2.4.38 (Debian)
WebServer to PHP Interface fpm-fcgi
Joomla! Version Joomla! 4.1.0 Stable [ Kuamini ] 15-February-2022 09:30 GMT
I traced the root of the problem to line 695 of file libraries\src\Table\Table.php
// Bind the source value, excluding the ignored fields.
foreach ($this->getProperties() as $k => $v)
{
// Only process fields not in the ignore array.
if (!\in_array($k, $ignore))
{
if (isset($src[$k])) //line 695
{
$this->$k = $src[$k];
}
}
}
The isset()
function does return false if $src[$k] exists, but contains null.
See:
https://www.php.net/manual/en/function.isset.php
isset() will return false when checking a variable that has been assigned to null. Also note that a null character ("\0") is not equivalent to the PHP null constant.
By using the 'array_key_exists' function, the existence of key $k in the $src array is properly assessed even when its value is null.
Suggested change:
// Bind the source value, excluding the ignored fields.
foreach ($this->getProperties() as $k => $v)
{
// Only process fields not in the ignore array.
if (!\in_array($k, $ignore))
{
if(array_key_exists($k, $src)) //new line 695
{
$this->$k = $src[$k];
}
}
}
Note:
There's also a workround: call a reset() before each save() to force the re-initialization of the table object properties. But it's wasteful.
Labels |
Added:
No Code Attached Yet
|
Sure - no need to ask. You can work on every issue
@aforgh Thank you for your issue. The proposed solution cannot be used as it is not backwards compatible. If we want such a change it has to happen in a major release. As you also found, the way to go for now is to call the reset()
function before calling the save function. This ensures that you are working with a clean table object. Running a foreach to save data could also mean that there are sets of data with different fields. In this case you could get the value of set 1 present in set 2, you would not want that either. Therefore it is better to reset the table before binding new values as you may end up with mixed data.
@roland-d
When saving an array of data that has been prepared in advance the problem only happens when there's a null value in such array. In that case I have values of set 1 improperly carried over to set 2.
If I use the modified function (with 'array_key_exists' instead of 'isset') there's no such problem.
@richard67 I read the discussion. The issue with the column that can or cannot allow null values is not exactly relevant to this issue. If I try to force a null value into a field that does not allow it I get an error, obviously. That's why I carefully chose the 'checked_out' field for my example, since that field allows null values.
But the problem here is that when I want to save a null value into a field that allows null values and I do not reset the Table object, I get data improperly carried over.
I hope someday this incosistency will be fixed but I understand the need to apply such a modification in a major release.
Labels |
Added:
bug
|
Can we consider this fix it in upcoming 4.4? Currently it is not possible to set a value of an existing object to NULL and this is sort of a bug... The proposed solution by aforgh to use array_key_exist() isof isset seems good to me imho..
Is it ok if i work on this issue?