IBX-3856: Changed Checkbox's value to never be considered empty

[mnocon]

Question	Answer
JIRA issue	IBX-3856
Type	bugfix
Target Ibexa version	v3.3
BC breaks	yes, not significant

Replacement for https://github.com/ezsystems/ezplatform-version-comparison/pull/81

I've:

1. implemented a failing test showing the desired behaviour (ab459ce)
2. implemented the actual change (the new test now passes) (e01db7a)
3. smuggled a small improvement to the codebase (9f21465) - the isEmptyValue(SPIValue $value) signature does not allow nulls, this removal is even suggested by PHPStorm.

In the original discussion there was this suggestion:

From my point of view, isEmptyValue for CheckboxFieldtype should check only for null value as default value have that value set.

But I'm not sure about it - the default value seems to be false and null seems is not even specified as an allowed value:
https://github.com/ezsystems/ezplatform-kernel/blob/1.3/eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/CheckboxConverter.php#L74
https://github.com/ezsystems/ezplatform-kernel/blob/1.3/eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/CheckboxConverter.php#L39
https://github.com/ezsystems/ezplatform-kernel/blob/1.3/eZ/Publish/Core/FieldType/Checkbox/Value.php#L16-L21

Changing the default value to null (and allowing the value to be bool|null) seems like asking for trouble, because 3rd party code might not be ready to deal with nulls in the value - that's why I went the "this field is never empty" way.

If accepted, I will prepare a PR removing this condition:
https://github.com/ezsystems/ezplatform-admin-ui/blob/e9f2b07f26a47f0158417ea0e2ddf4f0ea5436b7/src/bundle/Resources/views/themes/admin/content/content_view_fields.html.twig#L25

as it will be no longer needed.

I will gladly accept any feedback, as my kernel-fu is very, very basic 😄

Checklist:

• Provided PR description.
• Tested the solution manually.
• Provided automated test coverage.
• Checked that target branch is set correctly (master for features, the oldest supported for bugs).
• Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
• Asked for a review (ping @ezsystems/engineering-team).

[alongosz]

Suggested change

	public function testAddFieldDefinition()
	public function testAddFieldDefinition(): void

[alongosz]

Would like to see sanity check prior addFieldDefinition - assert count = 1

[mnocon]

Not sure how to do this - this test is a variation of the base one:

ezplatform-kernel/eZ/Publish/API/Repository/Tests/FieldType/BaseIntegrationTest.php

Lines 1009 to 1022 in 59ca143

	public function testAddFieldDefinition()
	{
	$content = $this->addFieldDefinition();
	$this->assertCount(2, $content->getFields());
	$this->assertTrue(
	$this->getRepository()->getFieldTypeService()->getFieldType(
	$this->getTypeName()
	)->isEmptyValue(
	$content->getFieldValue('data')
	)
	);
	}

But I had to change the assertion.

The method 'addFieldDefinition` first removes the fieldDefinition:

ezplatform-kernel/eZ/Publish/API/Repository/Tests/FieldType/BaseIntegrationTest.php

Line 985 in 59ca143

$content = $this->removeFieldDefinition();

and then adds it again

The flow inside addFieldDefinition looks like this then:

• The Content item is created (with 2 fields)
• the tested field definition is removed
• the tested field definition is added again

Current approach makes the $content variable available only after the call to addFieldDefinition finishes, I can't really add that sanity check without reworking this test.

I wanted to reuse the existing setup methods, but if you think it's good to add some custom setup for this test then I will do it

[alongosz]

Static methods should be called statically.

Suggested change

	$this->assertCount(2, $content->getFields());
	self::assertCount(2, $content->getFields());

[alongosz]

Suggested change

	$this->assertFalse(
	self::assertFalse(

[alongosz]

Readability, please extract

$fieldTypeService = $this->getRepository()->getFieldTypeService()

as the first call of the method. If you need to call it after addFieldDefinition then it should be called as getRepository(false) because the method has side effects.

[alongosz]

Suggested change

	public function testValueIsNeverEmpty(CheckboxValue $value)
	public function testValueIsNeverEmpty(CheckboxValue $value): void

[alongosz]

@ezsystems/documentation-team this is behavior change - CheckBox Field Type after the change is not going to be considered as empty in any case - it can be either true or false. This should be reflected on the Doc page for that Field Type.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[tomaszszopinski]

QA approved IbexaDXP 3.3 commerce.