DBComposite::writeToManipulation() is never called

[kinglozzer]

Affected Version

4.0.0 - 4.3.0

Description

Noticed this when implementing a custom DBComposite field type - before a DataObject is written, DBField::writeToManipulation() should be called against all database fields, but it isn’t called on DBComposite fields.

Looks like there was a subtle behavioural change in commit 11bbed4.

Before:

silverstripe-framework/ORM/DataObject.php

Lines 1255 to 1263 in f0dd9af

	// Check if this record pertains to this table, and
	// we're not attempting to reset the BaseTable->ID
	if( empty($this->changed[$fieldName])
	|| ($table === $baseTable && $fieldName === 'ID')
	|| (!self::has_own_table_database_field($class, $fieldName)
	&& !self::is_composite_field($class, $fieldName, false))
	) {
	continue;
	}

After:

silverstripe-framework/ORM/DataObject.php

Lines 1190 to 1200 in 11bbed4

	// we're not attempting to reset the BaseTable->ID
	// Ignore unchanged fields or attempts to reset the BaseTable->ID
	if (empty($this->changed[$fieldName]) || ($table === $baseTable && $fieldName === 'ID')) {
	continue;
	}
	// Ensure this field pertains to this table
	$specification = $schema->fieldSpec($class, $fieldName, ['dbOnly', 'uninherited']);
	if (!$specification) {
	continue;
	}

The config option 'dbOnly' (since replaced by the bitmask DataObjectSchema::DB_ONLY) shouldn’t be set here - as that explicitly excludes “virtual” (i.e. composite) fields.

Marking as low impact as the composite values are still saved - by virtue of DBComposite::setField() calling $record->setField() for each of the composite values

Related PRs

• FIX: Ensure DBComposite::writeToManipulation() is called on writes (fixes #8800) #8816
• DBComposite::writeToManipulation() is never called #10913

[lekoala]

This is still a thing. Any plans to fix it? Why is this PR closed #8816 ?

[michalkleiner]

Not sure why the PR was closed. We can rebase the branch, reopen and retarget the PR I guess.

[kinglozzer]

I think it was automatically closed when the target branch was deleted

[michalkleiner]

Probably. GitHub doesn't allow me to reopen it, so it probably needs to be edited first or recreated completely.

[lekoala]

:-) i'm just asking because i have a module with a composite field where this could be really handy

[kinglozzer]

I recreated it here but looks like it had a failing test: #9182. If someone else wants to recreate the PR and fix it up - or just pinch some of the code - then feel free 🙂

[beerbohmdo]

This is still broken in Silverstripe 5. DBMoney fields are not written to database.

[lekoala]

@kinglozzer i took the liberty to take your PR and fix it
#10913

[GuySartorelli]

PR merged. This will be automatically tagged once CI has finished running on the branch.