[4.0] Error when saving an object in JTable->save()

[korenevskiy]

I created a class

class JTableProducts extends JTable {
    public int $id;
    public string $name;
    public int $cost;
}

Loading JSON.
$object = json_decode($json, true);

Save data.

$product = new JTableProducts;
$product->save($object,'','id');

But data with empty values is saved to the database.

I spent a lot of time looking for the problem.
Even the site //php.net does not report PHP restrictions
The Table->bind() method has exactly the same behavior.

The fact is that the Table->GetProperties() method has the code get_object_vars().
But the get_object_vars() function has limitations.
For the function to work, it needs to get the property names with their own values. But there are no default values in the JTable Products class.
The Table->Bind() method and the Table->Save() method do not use the original property values at all, these methods only set new values with properties.
This problem is a bug of PHP developers. After all, it is obvious that int fields by definition should have a default value of 0.
Why do I need to set default property values if I'm going to explicitly override the properties (in Table->save() and Table->bind())?
I suggest using something else in the Table->GetProperties() method instead of get_object_vars().
However, the Table-set() method works in all cases, and the Table->save() method should also work in all binding cases.

[richard67]

The use of get_object_vars was added there with PR #25761 and will be back-integrated with #33633 into staging/3.9.x.

@Hackwar Since the original PR #25761 was yours: Could you check this issue here and let us know your thoughts on it? Thanks in advance.

[joomdonation]

I don't think the PRs are related. For this requirement, @korenevskiy has can implement check() method in the table class to check if certain fields has empty value and set default value for it in the way he wants. That's how we do in Joomla, see https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Table/Content.php#L214 for example

The problem here is that look like he does not want to do that and want Joomla to do all magic things to set default values for these fields automatically. From what I can see, it is not possible, at least for our current code.

[richard67]

@joomdonation So we close it as expected behaviour?

[joomdonation]

At least this is not an issue. Move to discussion if needed but from what I see, this is something we won't support.

[korenevskiy]

@joomdonation @richard67

Of course, this is solved with a crutch.
But joomla is not only a CMS, it is also a framework.
The Table, Factory, and Helpers classes are classes that make life easier.
I have already solved my problem, I have set the default values to 0 and ".
I suggest improving the FrameWork as a tool to make life easier.
.
PHP 8 introduces another level of typing in classes. What is the use of this innovation? If you then need to do unnecessary type checks.

[richard67]

We still have to support PHP versions lower than 8 with Joomla 4.

[korenevskiy]

@richard67
The framework must bind itself even for typed data. But it should also be able to work as before in 7.4 without typing.
This problem does not conflict with PHP 7.4
I didn't create this ISSUE because it's not solvable at all.
But because the framework should be of high quality, making life easier.

[richard67]

I move this to discussions.

[korenevskiy]

Then let's discuss the topic of how to do it, and not what it is necessary for.
I want to note that development in all areas is moving to typed data. Typescript confirms this. Support for typing data in Joomla should be as easy as without it.

[korenevskiy]

@richard67 @joomdonation

The Table->GetProperties() method inherits from CMSObject->GetProperties()

I propose the following solution.

edit method CMSObject->GetProperties() :

	public function getProperties($public = true)
	{
		if(empty($public)){
			return (array)$this;
		}
		
		$vars = array_merge(get_class_vars(get_class($this)),get_object_vars($this));

		foreach ($vars as $key => $value)
		{
			if ('_' == substr($key, 0, 1))
			{
				unset($vars[$key]);
			}
		} 

		return $vars;
	}

[HLeithner]

I don't get the problem, if you bind a value to the table only the set values are written to the database or not?

[korenevskiy]

I don't get the problem, if you bind a value to the table only the set values are written to the database or not?

Yes, write only empty values

[korenevskiy]

@HLeithner @richard67
But in fact, it would be logical to use the default values taken from the database table.

$fields = JTable->getFields(false);

But why does the constructor load fields from the database and their default values, but the constructor does not assign default values to the table fields?

[korenevskiy]

For untyped properties, it doesn't matter. But using a typed value, the class no longer functions with typed fields without values. Therefore, it makes sense to assign default values from the database.
And also if the database has default values, the developer will have to come up with a new method to assign these values from the database a second time.
Let's make an assignment in the class constructor.

[HLeithner]

Oh now I unterstand, I think jtable doesn't load the fields from the database normally only if requested or I'm wrong?

[korenevskiy]

Oh now I unterstand, I think jtable doesn't load the fields from the database normally only if requested or I'm wrong?

joomla-cms/libraries/src/Table/Table.php

Line 164 in 6639c07

$fields = $this->getFields();

Loads, uniquely loads. And the JTable->GetFields() method is called in the __construct().

@HLeithner @richard67

abstract class Table extends CMSObject implements TableInterface, DispatcherAwareInterface
{
	public function __construct($table, $key, DatabaseDriver $db, DispatcherInterface $dispatcher = null)
	{
		foreach ($this->getFields() as $field){
			$this->{$field->Field} = $field->Default;
		}
	}
}

This will make a big difference for typed properties. Since typed properties require a definition.

[korenevskiy]

@HLeithner @richard67 @joomdonation
I just found out. There is a problem.
The JTable->reset(); method also causes an error.
I have a typed field public DateTime $date;
When calling the JTable->reset(); method, the error is that the JTable->reset(); method assigns all fields a default value. But the $date property has the DateTime type and the method assigns it a string with the CURRENT_TIMESTAMP value.
Of course, a string value will cause an error.
.
But please agree. The date type is not a special type, this is in every 2nd table of any database.
I'm new to PHP typizations, and typing has been around for a long time. But what I am facing now, I understand that people do not use it at all and have never used it in Joomla.

Crutches will be installed, as long as it works, there is no question of typization the code..

[korenevskiy]

Recently watched a course on Vue.JS. The course said that people have 90% of the written code, repeated.
The framework serves to solve the problem of repeatable code.
Therefore, the conversion of a property with the DateTime type to a value store in the database should fall on the framework.

[joomdonation]

@korenevskiy Development for Joomla! is not easy compare with developing for yourself. When we introduce something new (especially at library level), we will have to make sure that it is backward compatible.

So if believe that you can develop a solution which works for both modern code and old/existing code, feel free to open a PR and we can discuss about it from there.

For your original issue, actually, there is a different way to address it:

• You do not have to define properties for each field in your table class.
• Define the default value for each fields in your database table
• Use something like the below code (where $data is an array contains the value for each fields which you want to store)

// Remove empty values as you want
$data = array_filter($data);
$row->bind($data);
$row->check();
$row->store();

Something like that should serve your purpose. No need to define individual property for each field in your table class, Joomla! handles it automatically. The empty value is removed from $data array, thus when you call $row->store(), the default value for each fields which are defined in your table is used for your record , so it works.

Personal, I would focus to get Joomla! 4 releases. Any improvements, we can work on after that.

[korenevskiy]

@joomdonation
I agree with you completely. But Joomla 4 is not out yet. So there are chances to do it right.
And about compatibility with the old code. We can make a choice for developers to use the old or new method of tables or by smart compatible with no choice.
But why did I mention the problem? I can certainly use properties without types. But I'm developing my own component and I need full typing. If I didn't need typing, then there wouldn't be a problem. But I want to keep up with the technology and write the right code. So I will use typed code.
I use the NetBeans IDE. It automatically monitors types and Auto IntelliSense works well.

Typing in programming plays an important role for the IDE.
There is no typing in Javascript, Javascript knows without a programmer where what type it uses programmatically. Only the programmer himself does not know where what type is.
Typing is necessary for the programmer, agree with me.

I would create a PR. But I have problems with GitHub. I'm just learning to use GitHub on my own, I'm confused, and have a hard time understanding how to do it.
I have 3 repositories of my own. I created them 3 months ago.

	private static function getTypeTableProperty($typeDataBaseField) {
			$typeDB = explode( '(', $typeDataBaseField);
			$typeDB = $typeDB[0];
			$type = ['int'=>'int','datetime'=>'string','varchar'=>'string','tinyint'=>'int','text'=>'string','json'=>'text','float'=>'float'];
			return $type[$typeDB];
	}

I use this code to convert the types in the table.
This is necessary so that empty fields or fields with 0 or an empty string are cast to the type in regardless of all the reasons.

[korenevskiy]

Serious, corporate, large programs are no longer written in JS. Programs write in TypeScript. Therefore, the Joomla FrameWork should also be typed in a modern way. If this is not the case, then serious applications will not be developed on the Joomla FrameWork.

[korenevskiy]

The method described above seems silly. But this is not the case. Such a method will be necessary, since the table must have a clear comparability to the types of the table and the database.

[korenevskiy]

@richard67 @joomdonation @HLeithner

Tell me please.
I'm going to add changes to the table class.
Changes will be made to support class typing.
I have done many tests and experiments. And made a good class for working with data typing.
For example, if the property has the DateTime type, the data loaded from the database will be converted to the JDate type, and vice versa, when saving the table class, properties with the JDate type will be converted to the STRING format, and automatically.
This transformation should be integrated into the Framework, since the framework is designed for this purpose. My changes will also add other improvements to support typing with other types.
My changes will affect 3 methods, __concstructor(), bind(), store(), GetFields().
And also add more methods toSqlData(), getTypeProp(), TypeConvert().
I decided to divide the typing extension into 2 stages (2 PR):

1. Adding an empty toSqlData() method, with called method AbstractEvent located in it. (the work of the class does not change)
2. Adding code that improves typization.
   However, the basic operation of the table class will NOT change.

Dear colleagues. Please tell me to which repository should I do PR?
4.0-dev or 4.1-dev
PS My changes concern the JTable(\Joomla\CMS\Table\Table) class .

[richard67]

@korenevskiy As far as I understand you it's for the framework, right? Then a PR should be made in this repository https://github.com/joomla-framework/database , and it it shall be for Joomla 4 only, it should be made for the2.0-dev branch of that repository.

[korenevskiy]

@korenevskiy As far as I understand you it's for the framework, right? Then a PR should be made in this repository https://github.com/joomla-framework/database , and it it shall be for Joomla 4 only, it should be made for the2.0-dev branch of that repository.

My changes concern the JTable(\Joomla\CMS\Table\Table) class .
There is no JTable class in your link
I realized it is not in the Framework JTable (\Joomla\CMS\Table \ Table) class . But the JTable definitely performs its function Framework .
Which repository to send changes to. Tell me please.

[richard67]

Well then it's for the cms repository here. If to do it for the 4.0-dev or the 4.10-dev branch depends on if the maintainers or the production leads regard it as a new feature or not. If you think it's a new feature, make the PR for the 4.1-dev branch. Otherwise make it for the 4.0-dev branch and we later can rebase it to 4.1-dev if we see it falls under the "new feature" policy.

[korenevskiy]

I will do this for 4.0-dev.
And typization will also be a distinctive feature of Joomla 4.

[korenevskiy]

@joomdonation @HLeithner

I will do this for 4.0-dev.
And typization will also be a distinctive feature of Joomla 4.

[korenevskiy]

@joomdonation @HLeithner
I created a new table class.
Please read the description here.
[4.0] Add support full typizations for base class Table.