joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.74k stars 3.64k forks source link

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

Closed korenevskiy closed 3 years ago

korenevskiy commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@joomdonation So we close it as expected behaviour?

joomdonation commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

korenevskiy commented 3 years ago

@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 commented 3 years ago

I move this to discussions.