No Code Attached Yet bug
avatar aforgh
aforgh
1 Jun 2022

Steps to reproduce the issue

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);
}

Expected result

When I run the test above, I expect that bind($data_2) sets 'checkout' property to null.

Actual result

The actual result is that the 'checkout' property remains set to 1.

System information (as much as possible)

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

Additional comments (proposed solution)

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.

avatar aforgh aforgh - open - 1 Jun 2022
avatar joomla-cms-bot joomla-cms-bot - change - 1 Jun 2022
Labels Added: No Code Attached Yet
avatar joomla-cms-bot joomla-cms-bot - labeled - 1 Jun 2022
avatar eeshaanSA
eeshaanSA - comment - 4 Jun 2022

Is it ok if i work on this issue?

avatar chmst
chmst - comment - 4 Jun 2022

Sure - no need to ask. You can work on every issue

avatar richard67
richard67 - comment - 5 Jun 2022

With the fix proposed here you might run into the issue to e other way round: you might have null values where they are not allowed. See discussion in PR #37983 .

avatar roland-d
roland-d - comment - 5 Jun 2022

@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.

avatar aforgh
aforgh - comment - 6 Jun 2022

@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.

avatar chmst chmst - change - 17 Feb 2023
Labels Added: bug
avatar chmst chmst - labeled - 17 Feb 2023
avatar Giuse69
Giuse69 - comment - 18 Aug 2023

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..

Add a Comment

Login with GitHub to post a comment