I just checked Mantis 2.1.0 and it happens there as well.

I just checked Mantis 2.1.0 and it happens there as well.

Right.
The same applies to check box fields

The problem is caused by the function that get executed when storing/restoring the values to/from database
'#function_value_to_database' => 'cfdef_prepare_list_value_to_database', '#function_database_to_value' => 'cfdef_prepare_list_database_to_value',

No time to have a deeper look at the moment.

The root cause is that bug_update.php does not compare old vs new value when building the list of custom fields to update [1].

The problem can be fixed there or in custom field API [2], by adding a comparison between old and new values; the latter is less efficient (we can avoid calling custom_field_set_value() completely, since we already know the values), but would ensure consistent behavior from SOAP API, so maybe we should do both.

[1] https://github.com/mantisbt/mantisbt/blob/release-2.1.0/bug_update.php#L307-L308
[2] https://github.com/mantisbt/mantisbt/blob/release-2.1.0/core/custom_field_api.php#L1308

PR https://github.com/mantisbt/mantisbt/pull/1035

@dregad, not sure if you are aware that it works for other custom field types because there is a comparison of old and new value in function history_log_event_direct [1]

The comparison fails in this case as we store the values by adding | at the begin and the end)

function cfdef_prepare_list_value_to_database( $p_value ) {
    if( '' == $p_value ) {
        return '';
    } else {
        return '|' . $p_value . '|';
    }
}

but compare after removing the |

function cfdef_prepare_list_database_to_value( $p_value ) {
    return rtrim( ltrim( $p_value, '|' ), '|' );
}

[1] https://github.com/mantisbt/mantisbt/blob/release-2.1.0/core/history_api.php#L78

EDIT (dregad) use Markdown `` instead of for proper formatting of the code

@atrol I did not realize that, thanks for pointing me to it. I'll have a look at the logic there.

That being said, I think it makes sense to merge the proposed change anyway, as there is no point in executing an UPDATE query when it's not needed.

I think the logic in the call to history_log_event_direct() in custom_field_set_value() is wrong.

1. $t_value contains the new field value, in DB format
2. old field value is converted to display value format

So history_log_event_direct() is effectively comparing apples with oranges.

@atrol your feedback on 0022408:0055747 would be appreciated. Do you agree with my analysis ?

I don't have time to have a deeper look and for tests.
So you think that call of history_log_event_direct has to be changed in function custom_field_set_value to

        history_log_event_direct( $p_bug_id, $t_name, $t_row[$t_value_field], $t_value );

I didn't test either, but yes this is what I think

@vboctor, what's your take on 0022408:0055747 ?

Hi,
This fix is OK for me further to testing in version 2.0.0.
Thanks a lot !

Hi,

i think the correct fix is in the function custom_field_set_value :

history_log_event_direct( $c_bug_id, $t_name, custom_field_database_to_value( $row['value'], $t_type ), $p_value );

So was this line in Mantis 1.2.10. So instead of $t_value it has to be $p_value, which is the new not formatted value.

@dregad seems reasonable since it seems to fix the issue :)

Hi! Any update on this issue? from the above history it looks like the PR is there but we're still seeing this issue so are there plans to roll it out?

@sandyj there is a good reason that the PR is not merged, see my comment

In file custom_field_api.php, line 1352:

history_log_event_direct( $p_bug_id, $t_name, custom_field_database_to_value( $t_row[$t_value_field], $t_type ), $t_value );

can we call a db_affected_rows before it to see if a history is needed to be inserted?

created a pull request: https://github.com/mantisbt/mantisbt/pull/1514

still happening in 2.23.0

and still happening in 2.25.2
@dregad will there be a fix for this 5 year old bug?

PR: https://github.com/mantisbt/mantisbt/pull/1930

We need a hero to fix this bug because it's crucial to preserving data integrity.

Hello, great job developing MantisBT, thank you for it.

As MarcoW suggested in 0022408:0055863 to fix the issue for me I replaced the last parameter in history_log_event_direct to $p_value, so correct values are compared.
https://github.com/mantisbt/mantisbt/blob/52281665e40b4976897b794fe7a99d3bde7cf37e/core/custom_field_api.php#L1512

Seems both PRs complicate the matter and it prevents the issue to be fixed. For example in PR 1930 an addition to core/custom_field_api.php doesn't seem right, $t_value is a value prepared for the database and for some time, checkbox and multiselect values are stored with a pipe at the beginning and the end, so it would break integrity of stored data.
In PR 1035 there's an issue with changed 1 to 1.0 not being logged to history.

So what about changing one character and fix it for now?

image.png (2,887 bytes)   

image.png (2,887 bytes)   

@pkaizar thanks for the ping, I had forgotten about this issue. Reading your comment, I realize that I completely overlooked @MarcoW's March 2017 note...

The change from $p_value to $p_value was made (by me !) in commit MantisBT master-1.3.x 4dcb16cc, as part of a fix for an unrelated MySQL issue (0021101). To be honest, in retrospect over 9 years after committing that change, I'm not really sure why I altered the history_log_event_direct() call to use $t_value. Maybe just a dumb replacement of all references to $p_value without considering the implications.

In any case you may be right that changing back to $p_value will fix this Issue, but some testing is needed to confirm.

@dregad as feedback after all those years, i'd like to say that mantisbt is active in our production with this little change i have mentioned without any problems. I have taken this change in different releases, now we're at 2.15.

Hello @dregad I tested it on version 2.25.2, so would be great, if someone could test it with the latest version.

On the image are updates to the page. COE is input, Location checkboxes.
Earlier than 16 hour I just clicked update and made no changes.
16:03 I changed only COE, Location wasn't changed, but got logged to history too.
Then $t_value to $p_value was changed in the code.
17:38 I changed COE again, no history entry for Location.

image-2.png (20,521 bytes)   

image-2.png (20,521 bytes)   

Well, those are 2 production tests, because we have it released too :)