N°3717 Improve iTop object history API (#192)

This fixes a major flaw in the history API that was causing "phantom" CMDBChange records (without any CMDBChangeOp attached). That was happening especially in iProcess impl.
For example this lead to the creation of the combodo-cmdbchange-cleaner module in the Mail To Ticket extension.

The modifications in detail : 
- We can now pass a non persisted CMDBChange instance to \CMDBObject::SetCurrentChange
- No persistence done in \CMDBObject::CreateChange anymore
- Persistence of the attached CMDChange will be done if necessary in CMDBChangeOp::OnInsert
- New CMDBObject::SetCurrentChangeFromParams helper method to ease resetting the current change

core/cmdbchangeop.class.inc.php

@@ -63,22 +63,30 @@ class CMDBChangeOp extends DBObject
 
 	/**
 	 * Describe (as a text string) the modifications corresponding to this change
-	 */	 
+	 */
 	public function GetDescription()
 	{
 		return '';
 	}
 
 	/**
-	 * Safety net: in case the change is not given, let's guarantee that it will
-	 * be set to the current ongoing change (or create a new one)	
-	 */	
+	 * Safety net:
+	 * * if change isn't persisted yet, use the current change and persist it if needed
+	 * * in case the change is not given, let's guarantee that it will be set to the current ongoing change (or create a new one)
+	 *
+	 * @since 2.7.7 3.0.2 3.1.0 N°3717 do persist the current change if needed
+	 */
 	protected function OnInsert()
 	{
-		if ($this->Get('change') <= 0)
-		{
-			$this->Set('change', CMDBObject::GetCurrentChange());
+		$iChange = $this->Get('change');
+		if (($iChange <= 0) || (is_null($iChange))) {
+			$oChange = CMDBObject::GetCurrentChange();
+			if ($oChange->IsNew()) {
+				$oChange->DBWrite();
+			}
+			$this->Set('change', $oChange);
 		}
+
 		parent::OnInsert();
 	}
 }

core/cmdbobject.class.inc.php

@@ -114,6 +114,26 @@ abstract class CMDBObject extends DBObject
 		self::$m_oCurrChange = $oChange;
 	}
 
+	/**
+	 * @param string $sUserInfo
+	 * @param string $sOrigin
+	 * @param \DateTime $oDate
+	 *
+	 * @throws \CoreException
+	 *
+	 * @since 2.7.7 3.0.2 3.1.0 N°3717 new method to reset current change
+	 */
+	public static function SetCurrentChangeFromParams($sUserInfo, $sOrigin = null, $oDate = null)
+	{
+		static::SetTrackInfo($sUserInfo);
+		static::SetTrackOrigin($sOrigin);
+		static::CreateChange();
+
+		if (!is_null($oDate)) {
+			static::$m_oCurrChange->Set("date", $oDate);
+		}
+	}
+
 	//
 	// Todo: simplify the APIs and do not pass the current change as an argument anymore
 	//       SetTrackInfo to be invoked in very few cases (UI.php, CSV import, Data synchro)
@@ -145,6 +165,8 @@ abstract class CMDBObject extends DBObject
 	 *    $oMyChange->Set("userinfo", 'this is done by ... for ...');
 	 *    $iChangeId = $oMyChange->DBInsert();
 	 *
+	 * **warning** : this will do nothing if current change already exists !
+	 *
 	 * @see SetCurrentChange to specify a CMDBObject instance instead
 	 *
 	 * @param string $sInfo
@@ -157,6 +179,8 @@ abstract class CMDBObject extends DBObject
 	/**
 	 * Provides information about the origin of the change
 	 *
+	 * **warning** : this will do nothing if current change already exists !
+	 *
 	 * @see SetTrackInfo
 	 * @see SetCurrentChange to specify a CMDBObject instance instead
 	 *
@@ -167,18 +191,15 @@ abstract class CMDBObject extends DBObject
 	{
 		self::$m_sOrigin = $sOrigin;
 	}
-	
+
 	/**
 	 * Get the additional information (defaulting to user name)
-	 */	 	
-	protected static function GetTrackInfo()
+	 */
+	public static function GetTrackInfo()
 	{
-		if (is_null(self::$m_sInfo))
-		{
+		if (is_null(self::$m_sInfo)) {
 			return CMDBChange::GetCurrentUserName();
-		}
-		else
-		{
+		} else {
 			return self::$m_sInfo;
 		}
 	}
@@ -201,7 +222,8 @@ abstract class CMDBObject extends DBObject
 	/**
 	 * Set to {@link $m_oCurrChange} a standard change record (done here 99% of the time, and nearly once per page)
 	 *
-	 * The CMDBChange is persisted so that it has a key > 0, and any new CMDBChangeOp can link to it
+	 * @since 2.7.7 3.0.2 3.1.0 N°3717 {@see CMDBChange} **will be persisted later** in {@see \CMDBChangeOp::OnInsert} (was done previously directly here)
+	 *     This will avoid creating in DB CMDBChange lines without any corresponding CMDBChangeOp
 	 */
 	protected static function CreateChange()
 	{
@@ -209,7 +231,6 @@ abstract class CMDBObject extends DBObject
 		self::$m_oCurrChange->Set("date", time());
 		self::$m_oCurrChange->Set("userinfo", self::GetTrackInfo());
 		self::$m_oCurrChange->Set("origin", self::GetTrackOrigin());
-		self::$m_oCurrChange->DBInsert();
 	}
 
 	/**

datamodels/2.x/combodo-db-tools/dbtools.php

@@ -301,9 +301,7 @@ function DisplayLostAttachments(iTopWebPage &$oP, ApplicationContext &$oAppConte
 					$sHistoryEntry = Dict::Format('DBTools:LostAttachments:History', $oOrmDocument->GetFileName());
 					CMDBObject::SetTrackInfo(UserRights::GetUserFriendlyName());
 					$oChangeOp = MetaModel::NewObject('CMDBChangeOpPlugin');
-					/** @var \Change $oChange */
-					$oChange = CMDBObject::GetCurrentChange();
-					$oChangeOp->Set('change', $oChange->GetKey());
+					// CMDBChangeOp.change will be automatically filled
 					$oChangeOp->Set('objclass', $sTargetClass);
 					$oChangeOp->Set('objkey', $sTargetId);
 					$oChangeOp->Set('description', $sHistoryEntry);

test/core/CMDBObjectTest.php

@@ -0,0 +1,75 @@
+<?php
+
+namespace Combodo\iTop\Test\UnitTest\Core;
+
+
+use CMDBObject;
+use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
+use MetaModel;
+
+/**
+ * @since 2.7.7 3.0.2 3.1.0 N°3717 tests history objects creation
+ *
+ * @package Combodo\iTop\Test\UnitTest\Core
+ */
+
+
+/**
+ * @runTestsInSeparateProcesses
+ * @preserveGlobalState disabled
+ * @backupGlobals disabled
+ */
+class CMDBObjectTest extends ItopDataTestCase
+{
+	/**
+	 * @covers CMDBObject::SetCurrentChange
+	 */
+	public function testCurrentChange()
+	{
+		// save initial conditions
+		$oInitialCurrentChange = CMDBObject::GetCurrentChange();
+		$sInitialTrackInfo = CMDBObject::GetTrackInfo();
+		// reset current change
+		CMDBObject::SetCurrentChange(null);
+
+		//-- new object with only track info
+		$sTrackInfo = 'PHPUnit test';
+		CMDBObject::SetTrackInfo($sTrackInfo);
+		/** @var \DocumentWeb $oTestObject */
+		$oTestObject = MetaModel::NewObject('DocumentWeb');
+		$oTestObject->Set('name', 'PHPUnit test');
+		$oTestObject->Set('org_id', 1);
+		$oTestObject->Set('url', 'https://www.combodo.com');
+		$oTestObject->DBWrite();
+		self::assertFalse(CMDBObject::GetCurrentChange()->IsNew(), 'TrackInfo : Current change persisted');
+		self::assertEquals($sTrackInfo, CMDBObject::GetCurrentChange()->Get('userinfo'),
+			'TrackInfo : current change created with expected trackinfo');
+
+		//-- new object with non persisted current change
+		$sTrackInfo2 = $sTrackInfo.'_2';
+		/** @var \CMDBChange $oCustomChange */
+		$oCustomChange = MetaModel::NewObject('CMDBChange');
+		$oCustomChange->Set('date', time());
+		$oCustomChange->Set('userinfo', $sTrackInfo2);
+		CMDBObject::SetCurrentChange($oCustomChange);
+		$oTestObject->Set('url', 'https://fr.wikipedia.org');
+		$oTestObject->DBUpdate();
+		self::assertFalse(CMDBObject::GetCurrentChange()->IsNew(), 'SetCurrentChange : Current change persisted');
+		self::assertEquals($sTrackInfo2, CMDBObject::GetCurrentChange()->Get('userinfo'),
+			'SetCurrentChange : current change created with expected trackinfo');
+
+		//-- new object with current change init using helper method
+		$sTrackInfo3 = $sTrackInfo.'_3';
+		CMDBObject::SetCurrentChangeFromParams($sTrackInfo3);
+		$oTestObject->Set('url', 'https://en.wikipedia.org');
+		$oTestObject->DBUpdate();
+		self::assertFalse(CMDBObject::GetCurrentChange()->IsNew(), 'SetCurrentChangeFromParams : Current change persisted');
+		self::assertEquals($sTrackInfo3, CMDBObject::GetCurrentChange()->Get('userinfo'),
+			'SetCurrentChangeFromParams : current change created with expected trackinfo');
+
+		// restore initial conditions
+		$oTestObject->DBDelete();
+		CMDBObject::SetCurrentChange($oInitialCurrentChange);
+		CMDBObject::SetTrackInfo($sInitialTrackInfo);
+	}
+}

webservices/cron.php

@@ -266,12 +266,9 @@ function CronExec($oP, $aProcesses, $bVerbose, $bDebug=false)
 
 			// N°3219 for each process will use a specific CMDBChange object with a specific track info
 			// Any BackgroundProcess can overrides this as needed
-			CMDBObject::SetCurrentChange(null);
-			CMDBObject::SetTrackInfo("Background task ($sTaskClass)");
-			CMDBObject::SetTrackOrigin(null);
+			CMDBObject::SetCurrentChangeFromParams("Background task ($sTaskClass)");
 
-			if (!array_key_exists($sTaskClass, $aTasks))
-			{
+			if (!array_key_exists($sTaskClass, $aTasks)) {
 				// New entry, let's create a new BackgroundTask record, and plan the first execution
 				$oTask = new BackgroundTask();
 				$oTask->SetDebug($bDebug);