FIX 16.0 - $object->updateCommon($user) = loss of fk_user_author

[atm-florianm]

FIX fk_user_author lost when calling updateCommon()

When createCommon() is called, the database column fk_user_author is correctly set using $user->id, but the object's field in memory (which, unfortunately, is named differently, namely `user) does not reflect that and remains NULL.

When updateCommon() is called, it uses setSaveQuery(), which sets the database column fk_user_author to 0 because $this->fk_user_author isn't set. Even if user_creation_id were correctly set, the naming difference would still cause fk_user_author to be updated to 0.

Here is a minimalist reproduction case:

<?php
chdir('{…}/dolibarr/htdocs');
include 'master.inc.php';
$user->fetch(1);
require_once DOL_DOCUMENT_ROOT.'/product/class/product.class.php';
$p = new Product($db);

$p->ref = 'test';
$p->type = 0;
$p->label = 'test';
$p->entity = 1;
$id = $p->create($user);

var_dump($p->user_creation_id); // Expected 1, Got NULL
$obj = $db->getRow('SELECT fk_user_author FROM llx_product WHERE rowid = '.$id);
var_dump($obj->fk_user_author); // Expected 1, Got 1
$res = $p->updateCommon($user);
$obj = $db->getRow('SELECT fk_user_author FROM llx_product WHERE rowid = '.$id);
var_dump($obj->fk_user_author); // Expected 1, Got 0

I am not sure that my fix is the right way of fixing the problem because I am not sure whether updateCommon() should or shouldn't modify fk_user_author if user_creation_id has been changed on the object. I assumed it should not be changed.

I also haven't investigated whether other fields with a naming difference between database column and the corresponding CommonObject field have a similar problem.

[eldy]

I pushed a different patch to fix this: 284fb49
Can you check if it is enough. Wea re in a transitionfrom fk_user_create and user_modification_id so both field must be managed.

[atm-florianm]

Can you check if it is enough.

It doesn't seem to fix the problem: when I re-ran my test after pulling 16.0, I obtained the same results as before : NULL, 1, 0 instead of 1, 1, 1.

[eldy]

Can you check if it is enough.

It doesn't seem to fix the problem: when I re-ran my test after pulling 16.0, I obtained the same results as before : NULL, 1, 0 instead of 1, 1, 1.

The first var_dump($p->user_creation_id);
will be NULL because the fields declared a property $fk_user_creat and not $user_creation_id. This is the expected result.
But what do you have if you do after the create
var_dump($p->fk_user_creat);

[atm-florianm]

But what do you have if you do after the create
var_dump($p->fk_user_creat);

I still get NULL, 1, 0 instead of 1, 1, 1.

[eldy]

But what do you have if you do after the create
var_dump($p->fk_user_creat);

I still get NULL, 1, 0 instead of 1, 1, 1.

NULL, 1, 0 is result for your sample test. But what do you have if you add
var_dump($p->fk_user_creat);
after the
$id = $p->create($user);

[atm-florianm]

Sorry for my previous answer: it was incomplete and out of context. I also realize there are problems with both my initial test case and my attempted fix, which made things confusing.

In fact there are two problems, which are related but which call for distinct fixes.

Problem 1

The first problem is that after calling createCommon(), the object in memory does not reflect the value of fk_user_author in database: both fk_user_creat and user_creation_id are empty (null).

• Affects classes that rely on createCommon(), typically classes made with ModuleBuilder.
• Also affects at least some core classes that don't rely on createCommon(), for instance Product::create() and Facture::create().

Problem 2

The second problem is that after calling updateCommon(), the row in database the object has its fk_user_author column set to 0. The fields on the object are empty too.

• Affects classes that rely on updateCommon().
• I think core classes that don't use updateCommon() are OK ; I tested Product::update() and Facture::update(): no problem here.

Updated test case

Here is an updated test case: it tests 4 cases:

1. create() method of class created with ModuleBuilder, relying on commonCreate()
2. update() method of class created with ModuleBuilder, relying on commonUpdate()
3. create() method of class Product (not created with ModuleBuilder, not using commonCreate())
4. update() method of class Product (not created with ModuleBuilder, not using commonUpdate())

Each test has 3 sub-tests:

1. field fk_user_creat of object has the expected value
2. field user_creation_id of object has the expected value
3. column fk_user_author of row in database has the expected value

require_once DOL_DOCUMENT_ROOT . '/product/class/product.class.php';
require_once DOL_DOCUMENT_ROOT . '/knowledgemanagement/class/knowledgerecord.class.php';
$TestResults = [];
$numTest = 1;

$langs->loadLangs(array('main', 'errors', 'knowledgemanagement', 'ticket', 'other'));
$db->begin();
$user->fetch(1);


// TEST USING A CORE CLASS CREATED WITH MODULEBUILDER
$object = new KnowledgeRecord($db);
$object->question = 'test';

$object->create($user);
$obj = $db->getRow('SELECT rowid, fk_user_creat FROM ' . $db->prefix() . $object->table_element . ' WHERE rowid = ' . $object->id);
$TestResults['TEST ' . $numTest++ . ' (CommonObject::createCommon)'] = [
    '$object->fk_user_creat == 1' => $object->fk_user_creat == 1,
    '$object->user_creation_id == 1' => $object->user_creation_id == 1,
    '$obj->fk_user_creat == 1' => $obj->fk_user_creat == 1
];

$object->update($user);
$obj = $db->getRow('SELECT rowid, fk_user_creat FROM ' . $db->prefix() . $object->table_element . ' WHERE rowid = ' . $object->id);
$TestResults['TEST ' . $numTest++ . ' (CommonObject::updateCommon)'] = [
    '$object->fk_user_creat == 1' => $object->fk_user_creat == 1,
    '$object->user_creation_id == 1' => $object->user_creation_id == 1,
    '$obj->fk_user_creat == 1' => $obj->fk_user_creat == 1
];


// TEST USING A CORE CLASS NOT CREATED WITH MODULEBUILDER
$p = new Product($db);
$p->ref = 'test';
$p->type = 0; // produit
$p->label = 'test';
$p->entity = 1;
$id = $p->create($user);

$obj = $db->getRow('SELECT rowid, fk_user_author FROM ' . $db->prefix() . $p->table_element . ' WHERE rowid = ' . $p->id);
$TestResults['TEST ' . $numTest++ . ' (Product::create)'] = [
    '$p->user_creation_id == 1' => $p->user_creation_id == 1,
    '$p->fk_user_creat == 1' => $p->fk_user_creat == 1,
    '$obj->fk_user_author == 1' => $obj->fk_user_author];

$p->update($p->id, $user);
$obj = $db->getRow('SELECT rowid, fk_user_author FROM ' . $db->prefix() . $p->table_element . ' WHERE rowid = ' . $p->id);
$TestResults['TEST ' . $numTest++ . ' (Product::update)'] = [
    '$p->user_creation_id == 1' => $p->user_creation_id == 1,
    '$p->fk_user_creat == 1' => $p->fk_user_creat == 1,
    '$obj->fk_user_author == 1' => $obj->fk_user_author];

echo json_encode($TestResults, JSON_PRETTY_PRINT);
$db->rollback();

These are the results with today's 16.0.

• I see that your commit 284fb49 does fix 1.1 and 2.1. 🎉
• I tried it with my PR branch: no tests pass that would not have passed before, so my fix fixes nothing.
• We can see that the 3rd subtest for Product is always OK
• All other tests remain to be fixed.

{
    "TEST 1 (CommonObject::createCommon)": {
        "$object->fk_user_creat == 1": true,
        "$object->user_creation_id == 1": false,
        "$obj->fk_user_author == 1": true
    },
    "TEST 2 (CommonObject::updateCommon)": {
        "$object->fk_user_creat == 1": true,
        "$object->user_creation_id == 1": false,
        "$obj->fk_user_author == 1": true
    },
    "TEST 3 (Product::create)": {
        "$p->user_creation_id == 1": false,
        "$p->fk_user_creat == 1": false,
        "$obj->fk_user_author == 1": "1"
    },
    "TEST 4 (Product::update)": {
        "$p->user_creation_id == 1": false,
        "$p->fk_user_creat == 1": false,
        "$obj->fk_user_author == 1": "1"
    }
}

Solutions / ideas

• 1.1 and 2.1: already fixed by your commit
• 1.2 and 2.2: something similar to what you did to fix 1.1 and 2.1
• 1.3 and 2.3: setSaveQuery() searches for $fieldvalues['fk_user_author'] but we set $fieldvalues['fk_user_creat']… maybe we could also set $fieldvalues['fk_user_author'] ? [edit] you already fixed it
• 3.1, 3.2, 4.1 and 4.2: specific to Product::create() and Product::update(), maybe just $this->user_creation_id = $this->fk_user_creat = $user->id somewhere in the functions ? + check if other core classes have similar problems.

I'll try to update my PR accordingly.

[atm-florianm]

Sorry, I again made a mistake: I thought that ModuleBuilder created a database column named fk_user_author corresponding to the object fieldfk_user_creat, but I realized the column is actually called fk_user_creat (fk_user_author is for tables of older objects not using modulebuilder).

I'm updating the test case in my previous comment.

[eldy]

About your test, it miss an important information. the field ->xxx for user creation must be filled in memory like it is after the create (because it is defined by the create itself), but only if the property has been defined on the class.
If class does not have property ->user_creation_id, field should not be filled.
Same for fk_user_creat.

In v16, there is a lot of object than as only 1 among the 3 property.

So can you must the test to say on which object you did the test because result may differ.
Note: In higher version, both fields are more generally declared with the other with status deprecated, so we will be able to manage both of them until a transition is completed and we will be able to remove one of the two everywhere. But in v16, fields sometimes are not a field of the object. In such a case, only the one declared must/can be filled.

[atm-florianm]

I am closing my PR because the main problem has been fixed, and the remaining improvements will be better treated in individual PRs: this will save everyone from having to read my lengthy and mostly pointless comments. 📚