From a34274b8831687fb35c8de9045445d5a6fa8ac41 Mon Sep 17 00:00:00 2001 From: Molkobain Date: Tue, 7 Mar 2023 10:16:14 +0100 Subject: [PATCH] =?UTF-8?q?N=C2=B05784=20-=20PHP=208.0:=20Fix=20mandatory?= =?UTF-8?q?=20attribute=20not=20visible=20in=20transition=20form=20due=20t?= =?UTF-8?q?o=20bad=20emptiness=20test=20(#379)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * N°5784 - PHP 8.0: Fix mandatory attribute not visible in transition form due to bad emptiness test * N°5784 - Rework AttributeDefinition::HasAValue() implementation after code review * N°5784 - Add unit test --- application/cmdbabstract.class.inc.php | 5 +- core/attributedef.class.inc.php | 177 ++++++++++++++++++ core/dbobject.class.php | 14 ++ .../core/AttributeDefTest.inc.php | 144 ++++++++++++++ 4 files changed, 338 insertions(+), 2 deletions(-) diff --git a/application/cmdbabstract.class.inc.php b/application/cmdbabstract.class.inc.php index 31ae27075..7193a4f2a 100644 --- a/application/cmdbabstract.class.inc.php +++ b/application/cmdbabstract.class.inc.php @@ -3374,13 +3374,14 @@ EOF // Consider only the "expected" fields for the target state if (array_key_exists($sAttCode, $aExpectedAttributes)) { $iExpectCode = $aExpectedAttributes[$sAttCode]; + // Prompt for an attribute if // - the attribute must be changed or must be displayed to the user for confirmation // - or the field is mandatory and currently empty if (($iExpectCode & (OPT_ATT_MUSTCHANGE | OPT_ATT_MUSTPROMPT)) || - (($iExpectCode & OPT_ATT_MANDATORY) && ($this->Get($sAttCode) == ''))) { - $oAttDef = MetaModel::GetAttributeDef($sClass, $sAttCode); + (($iExpectCode & OPT_ATT_MANDATORY) && (false === $this->HasAValue($sAttCode)))) { $aArgs = array('this' => $this); + $oAttDef = MetaModel::GetAttributeDef($sClass, $sAttCode); // If the field is mandatory, set it to the only possible value if ((!$oAttDef->IsNullAllowed()) || ($iExpectCode & OPT_ATT_MANDATORY)) { if ($oAttDef->IsExternalKey()) { diff --git a/core/attributedef.class.inc.php b/core/attributedef.class.inc.php index 30c72ebb0..4692ff52b 100644 --- a/core/attributedef.class.inc.php +++ b/core/attributedef.class.inc.php @@ -705,6 +705,18 @@ abstract class AttributeDefinition return is_null($proposedValue); } + /** + * @param mixed $proposedValue + * + * @return bool True if $proposedValue is an actual value set in the attribute, false is the attribute remains "empty" + * @since 3.0.3, 3.1.0 N°5784 + */ + public function HasAValue($proposedValue): bool + { + // Default implementation, we don't really know what type $proposedValue will be + return is_null($proposedValue); + } + /** * force an allowed value (type conversion and possibly forces a value as mySQL would do upon writing! * @@ -1384,6 +1396,15 @@ class AttributeDashboard extends AttributeDefinition { return ''; } + + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + // Always return false for now, we don't consider a custom version of a dashboard + return false; + } } /** @@ -2233,6 +2254,22 @@ class AttributeLinkedSet extends AttributeDefinition { return false; } + + /** + * @inheritDoc + * @param \ormLinkSet $proposedValue + */ + public function HasAValue($proposedValue): bool + { + // Protection against wrong value type + if (false === ($proposedValue instanceof ormLinkSet)) + { + return parent::HasAValue($proposedValue); + } + + // We test if there is at least 1 item in the linkset (new or existing), not if an item is being added to it. + return $proposedValue->Count() > 0; + } } /** @@ -2614,6 +2651,14 @@ class AttributeInteger extends AttributeDBField return is_null($proposedValue); } + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + return utils::IsNotNullOrEmptyString($proposedValue); + } + public function MakeRealValue($proposedValue, $oHostObj) { if (is_null($proposedValue)) @@ -2713,6 +2758,14 @@ class AttributeObjectKey extends AttributeDBFieldVoid return ($proposedValue == 0); } + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + return ((int) $proposedValue) !== 0; + } + public function MakeRealValue($proposedValue, $oHostObj) { if (is_null($proposedValue)) @@ -2912,6 +2965,14 @@ class AttributeDecimal extends AttributeDBField return is_null($proposedValue); } + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + return utils::IsNotNullOrEmptyString($proposedValue); + } + public function MakeRealValue($proposedValue, $oHostObj) { if (is_null($proposedValue)) @@ -3323,6 +3384,14 @@ class AttributeString extends AttributeDBField return ($proposedValue == ''); } + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + return utils::IsNotNullOrEmptyString($proposedValue); + } + public function MakeRealValue($proposedValue, $oHostObj) { if (is_null($proposedValue)) @@ -4478,6 +4547,22 @@ class AttributeCaseLog extends AttributeLongText return ($proposedValue->GetText() == ''); } + /** + * @inheritDoc + * @param \ormCaseLog $proposedValue + */ + public function HasAValue($proposedValue): bool + { + // Protection against wrong value type + if (false === ($proposedValue instanceof ormCaseLog)) { + return parent::HasAValue($proposedValue); + } + + // We test if there is at least 1 entry in the log, not if the user is adding one + return $proposedValue->GetEntryCount() > 0; + } + + public function ScalarToSQL($value) { if (!is_string($value) && !is_null($value)) @@ -6798,6 +6883,14 @@ class AttributeExternalKey extends AttributeDBFieldVoid return ($proposedValue == 0); } + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + return ((int) $proposedValue) !== 0; + } + public function MakeRealValue($proposedValue, $oHostObj) { if (is_null($proposedValue)) @@ -7530,6 +7623,16 @@ class AttributeExternalField extends AttributeDefinition return $oExtAttDef->IsNull($proposedValue); } + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + $oExtAttDef = $this->GetExtAttDef(); + + return $oExtAttDef->HasAValue($proposedValue); + } + public function MakeRealValue($proposedValue, $oHostObj) { $oExtAttDef = $this->GetExtAttDef(); @@ -8116,6 +8219,20 @@ class AttributeBlob extends AttributeDefinition return $oFormField; } + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + if (false === ($proposedValue instanceof ormDocument)) { + return parent::HasAValue($proposedValue); + } + + // Empty file (no content, just a filename) are supported since PR {@link https://github.com/Combodo/combodo-email-synchro/pull/17}, so we check for both empty content and empty filename to determine that a document has no value + return utils::IsNotNullOrEmptyString($proposedValue->GetData()) && utils::IsNotNullOrEmptyString($proposedValue->GetFileName()); + } + + } /** @@ -9142,6 +9259,17 @@ class AttributeStopWatch extends AttributeDefinition return $sRet; } + + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + // A stopwatch always has a value + return true; + } + + } /** @@ -9627,6 +9755,19 @@ class AttributeOneWayPassword extends AttributeDefinition implements iAttributeN return '*****'; } + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + // Protection against wrong value type + if (false === ($proposedValue instanceof ormPassword)) { + return parent::HasAValue($proposedValue); + } + + return $proposedValue->IsEmpty() !== false; + } + } // Indexed array having two dimensions @@ -9676,6 +9817,15 @@ class AttributeTable extends AttributeDBField return (count($proposedValue) == 0); } + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + return count($proposedValue) > 0; + } + + public function GetEditValue($sValue, $oHostObj = null) { return ''; @@ -10197,6 +10347,18 @@ abstract class AttributeSet extends AttributeDBFieldVoid return $proposedValue->Count() == 0; } + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + if (false === ($proposedValue instanceof ormSet)) { + return parent::HasAValue($proposedValue); + } + + return $proposedValue->Count() > 0; + } + /** * To be overloaded for localized enums * @@ -12904,6 +13066,21 @@ class AttributeCustomFields extends AttributeDefinition return $bEquals; } + + /** + * @inheritDoc + */ + public function HasAValue($proposedValue): bool + { + // Protection against wrong value type + if (false === ($proposedValue instanceof ormCustomFieldsValue)) { + return parent::HasAValue($proposedValue); + } + + return count($proposedValue->GetValues()) > 0; + } + + } class AttributeArchiveFlag extends AttributeBoolean diff --git a/core/dbobject.class.php b/core/dbobject.class.php index 1a1fc51f3..147764f93 100644 --- a/core/dbobject.class.php +++ b/core/dbobject.class.php @@ -4001,6 +4001,20 @@ abstract class DBObject implements iDisplay return $bSuccess; } + /** + * @param string $sAttCode + * + * @return bool True if $sAttCode has an actual value set, false is the attribute remains "empty" + * @throws \ArchivedObjectException + * @throws \CoreException + * @since 3.0.3, 3.1.0 N°5784 + */ + public function HasAValue(string $sAttCode): bool + { + $oAttDef = MetaModel::GetAttributeDef(get_class($this), $sAttCode); + return $oAttDef->HasAValue($this->Get($sAttCode)); + } + /** * Helper to recover the default value (aka when an object is being created) * Suitable for use as a lifecycle action diff --git a/tests/php-unit-tests/unitary-tests/core/AttributeDefTest.inc.php b/tests/php-unit-tests/unitary-tests/core/AttributeDefTest.inc.php index 62bc0251d..fa70c5e3b 100644 --- a/tests/php-unit-tests/unitary-tests/core/AttributeDefTest.inc.php +++ b/tests/php-unit-tests/unitary-tests/core/AttributeDefTest.inc.php @@ -29,4 +29,148 @@ class AttributeDefTest extends ItopDataTestCase { $this->assertEquals(["status" => "ENUM('active','inactive') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci"], $aImportColumns); } + + /** + * @dataProvider HasAValueProvider + * @covers AttributeDefinition::HasAValue + * + * @param $sObjectClass + * @param $sAttCode + * @param $sUpdateCode + * @param $bHasAValueInitially + * @param $bHasAValueOnceSet + * + * @return void + * @throws \ArchivedObjectException + * @throws \CoreException + */ + public function testHasAValue($sObjectClass, $sAttCode, $sUpdateCode, $bHasAValueInitially, $bHasAValueOnceSet) + { + $oObject = MetaModel::NewObject($sObjectClass); + + // Test attribute without a value yet + $this->assertEquals($bHasAValueInitially, $oObject->HasAValue($sAttCode)); + + eval($sUpdateCode); + + // Test attribute once a value has been set + $this->assertEquals($bHasAValueOnceSet, $oObject->HasAValue($sAttCode)); + } + + public function HasAValueProvider(): array + { + // Note: This is test is not great as we are datamodel dependent and don't have a class with all the attribute types + return [ + 'AttributeDashboard' => [ + 'Organization', + 'overview', + '', + false, + false, + ], + 'AttributeLinkedSet' => [ + 'UserRequest', + 'workorders_list', + <<Get('workorders_list'); +\$ormLinkset->AddItem(MetaModel::NewObject('WorkOrder', [])); +\$oObject->Set('workorders_list', \$ormLinkset); +PHP, + false, + true, + ], + 'AttributeLinkedSetIndirect' => [ + 'UserRequest', + 'contacts_list', + <<Get('contacts_list'); +\$ormLinkset->AddItem(MetaModel::NewObject('Person', [])); +\$oObject->Set('contacts_list', \$ormLinkset); +PHP, + false, + true, + ], + 'AttributeInteger' => [ + 'SLT', + 'value', + <<Set('value', 100); +PHP, + false, + true, + ], + 'AttributeDecimal' => [ + 'PhysicalInterface', + 'speed', + <<Set('speed', 1024.5); +PHP, + false, + true, + ], + 'AttributeString' => [ + 'UserRequest', + 'title', + <<Set('title', 'Some title'); +PHP, + false, + true, + ], + 'AttributeObjectKey' => [ + 'Attachment', + 'item_id', + <<Set('item_id', 12); +PHP, + false, + true, + ], + 'AttributeExternalKey' => [ + 'UserRequest', + 'org_id', + <<Set('org_id', 3); +PHP, + false, + true, + ], + 'AttributeBlob' => [ + 'DocumentFile', + 'file', + <<Set('file', new ormDocument('something', 'text/plain', 'something.txt')); +PHP, + false, + true, + ], + 'AttributeStopWatch' => [ + 'UserRequest', + 'tto', + '', + true, + true, + ], + 'AttributeSubItem' => [ + 'UserRequest', + 'tto_escalation_deadline', + '', + true, + true, + ], + 'AttributeOneWayPassword' => [ + 'UserLocal', + 'password', + <<Set('password', \$ormPassword); +PHP, + false, + true, + ], + ]; + } } \ No newline at end of file