Merging contacts drops date custom fields from the secondary record

[donnchawp]

Quick summary

Merging two contacts silently drops a date custom field that only the secondary record has. The merge is supposed to patch empty fields on the main record from the secondary one, and it does that for every other field type. Date custom fields go missing without any error or log entry.

This is the same double normalisation as #21, reached without going anywhere near the API. It's worth recording separately because a fix confined to the API layer, like #27, doesn't reach it.

Steps to reproduce

1. In Jetpack CRM → Settings → Custom Fields, add a Contact custom field of type Date (e.g. label Contract Date, slug contract-date).
2. Create contact A and leave the date custom field empty.
3. Create contact B and set the date custom field to a real date.
4. Merge B into A, with A as the main record.

Expected: A ends up with B's date, the same as any other field the main record was missing. The merge log says "Copied field ... from secondary record over main record, (main was empty)."

Actual: A's date custom field is still empty. The merge reports the copy as a change, so the log claims it happened.

Root cause

zeroBSCRM_mergeCustomers() loads the main record with zeroBS_getCustomer(), which returns date custom fields as the raw UTS the DAL stores:

$master        = zeroBS_getCustomer( $dominantID );   // includes/ZeroBSCRM.DAL3.Helpers.php:1303
$masterNewMeta = $master;                             // :1343

The patch loop then copies the secondary record's value in, also a raw UTS:

$masterNewMeta[ $fieldPrefix . $fieldKey ] = $slave[ $fieldKey ];   // :1362

and the whole array goes to zeroBS_addUpdateCustomer() at :1471, which normalises it again with $removeEmpties on:

$zbsCustomerMeta = zeroBS_buildContactMeta( $cFields, $existingMeta, $metaBuilderPrefix, '', true );   // :1863

The date branch of the builder expects Y-m-d, gets "1784764800", and DateTime::createFromFormat( '!Y-m-d', '1784764800' ) fails. jpcrm_date_str_to_uts() returns false, $removeEmpties drops the key at :3049, and addUpdateContact() skips absent custom fields at ZeroBSCRM.DAL3.Obj.Contacts.php:3087. Nothing is written and nothing complains.

Three callers hand the builder an array that has already been through it: api/create_customer.php:51, api/create_company.php:30, and this one. Everything else normalises once and goes straight to addUpdateContact().

Fixed by #25

The idempotency fix in #25 covers this, because it makes the date branch pass an already-converted timestamp through untouched. Verified both ways:

Trunk:  ✘ Merging contacts copies a date custom field from the secondary record.
          Failed asserting that '' matches expected 1784764800.
Branch: OK (1 test, 2 assertions)

Filing it anyway so the case is recorded, since it's the clearest argument that the builder is the right place to fix this rather than the API endpoints.

The regression test is blocked

I've written the test but left it out of #25. It trips a pre-existing deprecation, and phpunit.11.xml.dist sets failOnDeprecation="true", so the suite exits 1 with it in:

ZeroBSCRM.DAL3.ObjectLayer.php:934
strlen(): Passing null to parameter #1 ($string) of type string is deprecated
Tests: 53, Assertions: 222, Deprecations: 1

The null values are tw, fb and li. A contact created without socials has NULL in those columns, getContact() hands the nulls back, and the merge passes them straight into the max-length check:

if ( strlen( $val ) > $this->objectModel[ $fieldKey ]['max_len'] ) {

It fires identically on trunk and on the #25 branch, so it has nothing to do with dates. Setting the socials in the test fixture doesn't avoid it, the nulls originate inside the merge.

Two ways to unblock the test, and I don't have a strong view on which:

• Cast at the check, strlen( (string) $val ). One line, and it fixes every other caller that round-trips a contact through addUpdateContact().
• Stop the merge handing null socials down in the first place, which is the real bug but a bigger change.

The test itself is ready and I can attach it to whichever gets picked.

Related

• API create_customer drops date custom fields (double date parse) #21, the original report, contacts and companies via the API
• Fix date custom fields being dropped by the create/update API #25, the idempotency fix
• Fix partial customer updates in legacy API #27, the create_customer.php rework

[donnchawp]

Here's the regression test, so it doesn't get lost. Goes at tests/php/entities/Merge_Contacts_Date_Test.php.

It depends on register_custom_field(), which landed on the #25 branch in 5cd683d (tests/php/class-jpcrm-base-integration-testcase.php). Custom fields are stored twice, once per object type for DAL3 and once in the legacy customfields setting, and zeroBSCRM_unpackCustomFields() gates the DAL3 overload on the object key existing in the legacy one, so both are needed before the field reaches the field globals.

As it stands this fails the suite on the strlen() deprecation described above, not on its own assertions. Unblock that first.

<?php

namespace Automattic\Jetpack\CRM\Entities\Tests;

use Automattic\Jetpack\CRM\Tests\JPCRM_Base_Integration_TestCase;
use PHPUnit\Framework\Attributes\TestDox;

/**
 * Coverage for date custom fields during a contact merge.
 *
 * zeroBSCRM_mergeCustomers() loads the master contact with zeroBS_getCustomer(), patches any
 * empty master fields from the slave (ZeroBSCRM.DAL3.Helpers.php:1362), and hands the result to
 * zeroBS_addUpdateCustomer(), which normalises it again with $removeEmpties on. Date custom
 * fields come back out of the DAL as a raw UTS, so the merge hits the same double normalisation
 * as the create/update API, without going anywhere near the API layer.
 *
 * @covers ::zeroBSCRM_mergeCustomers
 */
class Merge_Contacts_Date_Test extends JPCRM_Base_Integration_TestCase {

	/**
	 * Slug of the date custom field registered against contacts.
	 */
	private const FIELD_SLUG = 'contract-date';

	/**
	 * The date as the DAL stores it.
	 */
	private const DATE_UTS = 1784764800;

	/**
	 * Register a real date custom field against contacts.
	 *
	 * @return void
	 */
	public function set_up(): void {
		parent::set_up();

		$this->register_custom_field(
			self::FIELD_SLUG,
			'date',
			'Contract Date',
			array( 'customers' => ZBS_TYPE_CONTACT )
		);
	}

	/**
	 * Merging a contact has to carry a date custom field over from the slave when the master
	 * has none.
	 *
	 * @return void
	 */
	#[TestDox( 'Merging contacts copies a date custom field from the secondary record.' )]
	public function test_merge_copies_date_custom_field_from_slave() {
		global $zbs;

		$master_id = $this->add_contact(
			array(
				'email' => 'merge-master@example.com',
				'fname' => 'Master',
			)
		);

		$slave_id = $this->add_contact(
			array(
				'email'          => 'merge-slave@example.com',
				'fname'          => 'Slave',
				self::FIELD_SLUG => self::DATE_UTS,
			)
		);

		// Confirm the fixture is what the test thinks it is before merging.
		$slave = $zbs->DAL->contacts->getContact( $slave_id, array( 'withCustomFields' => true ) ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase
		$this->assertEquals(
			self::DATE_UTS,
			$slave[ self::FIELD_SLUG ],
			'The secondary contact did not have the date custom field to begin with.'
		);

		zeroBSCRM_mergeCustomers( $master_id, $slave_id );

		$master = $zbs->DAL->contacts->getContact( $master_id, array( 'withCustomFields' => true ) ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase

		$this->assertEquals(
			self::DATE_UTS,
			$master[ self::FIELD_SLUG ],
			'The date custom field was not copied from the secondary record during the merge.'
		);
	}
}

To run it:

make up
docker exec -w /var/www/html/wp-content/plugins/jetpack-crm \
  -e WORDPRESS_DEVELOP_DIR=/wordpress-phpunit zero-bs-crm-tests-cli-1 \
  sh -c 'php -d auto_prepend_file=tests/suppress_php84_deprecations.php \
  vendor/bin/phpunit -c phpunit.12.xml.dist --filter Merge_Contacts_Date_Test'

make test doesn't work inside the container, phpunit-select-config needs pcntl_exec and the wp-env CLI image doesn't have it, hence calling phpunit directly with the config named by hand.