N°2982 - fix delta xml variables not used in themes

application/themehandler.class.inc.php

@@ -215,7 +215,7 @@ class ThemeHandler
 	 * @param boolean $bSetup
 	 * @param string $sSetupCompilationTimestamp : setup compilation timestamp in micro secunds
 	 * @param array|null $aThemeParameters Parameters (variables, imports, stylesheets) for the theme, if not passed, will be retrieved from compiled DM
-	 * @param array|null $aImportsPaths Paths where imports can be found. Must end with '/'
+	 * @param array|null $aImportsPaths Folder paths where imports can be found. Must end with '/'
 	 * @param string|null $sWorkingPath Path of the folder used during compilation. Must end with a '/'
 	 *
 	 * @throws \CoreException
@@ -342,11 +342,12 @@ CSS;
 					static::$oCompileCSSService = new CompileCSSService();
 				}
 				//store it again to change $version with latest compiled time
+				SetupLog::Info("Compiling theme $sThemeId...");
 				$sTmpThemeCssContent = static::$oCompileCSSService->CompileCSSFromSASS($sTmpThemeScssContent, $aImportsPaths,
 					$aThemeParametersWithVersion);
+				SetupLog::Info("$sThemeId theme compilation done.");
 				file_put_contents($sThemeFolderPath.'/theme-parameters.json', json_encode($aThemeParameters));
 				file_put_contents($sThemeCssPath, $sSignatureComment.$sTmpThemeCssContent);
-				SetupLog::Info("Theme $sThemeId file compiled.");
 				return true;
 			}
 		}
@@ -939,7 +940,8 @@ CSS;
 			{
 				$aThemeParametersVariable = array_merge([], $aThemeParameters['variables']);
 			}
-		}		
+		}
+
 		if (array_key_exists('imports_variable', $aThemeParameters))
 		{
 			if (is_array($aThemeParameters['imports_variable']))

application/themehandlerservice.class.inc.php

@@ -0,0 +1,35 @@
+<?php
+/**
+ * Copyright (C) 2013-2020 Combodo SARL
+ *
+ * This file is part of iTop.
+ *
+ * iTop is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * iTop is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ */
+
+/**
+ * Class ThemeHandlerService : used to ease testing MFCompiler::CompileThemes class via mocks
+ *
+ * @author Olivier DAIN <olivier.dain@combodo.com>
+ * @since 3.0.0 N°2982
+ */
+class ThemeHandlerService
+{
+	public function __construct()
+	{
+	}
+
+	public function CompileTheme($sThemeId, $bSetup = false, $sSetupCompilationTimestamp="", $aThemeParameters = null, $aImportsPaths = null, $sWorkingPath = null){
+		return ThemeHandler::CompileTheme($sThemeId, $bSetup, $sSetupCompilationTimestamp="", $aThemeParameters, $aImportsPaths, $sWorkingPath);
+	}
+}

lib/composer/autoload_classmap.php

@@ -1916,6 +1916,7 @@ return array(
     'TemplateString' => $baseDir . '/core/templatestring.class.inc.php',
     'TemplateStringPlaceholder' => $baseDir . '/core/templatestring.class.inc.php',
     'ThemeHandler' => $baseDir . '/application/themehandler.class.inc.php',
+	'ThemeHandlerService' => $baseDir . '/application/themehandlerservice.class.inc.php',
     'ToolsLog' => $baseDir . '/core/log.class.inc.php',
     'Trigger' => $baseDir . '/core/trigger.class.inc.php',
     'TriggerOnObject' => $baseDir . '/core/trigger.class.inc.php',

lib/composer/autoload_static.php

@@ -2146,6 +2146,7 @@ class ComposerStaticInit0018331147de7601e7552f7da8e3bb8b
         'TemplateString' => __DIR__ . '/../..' . '/core/templatestring.class.inc.php',
         'TemplateStringPlaceholder' => __DIR__ . '/../..' . '/core/templatestring.class.inc.php',
         'ThemeHandler' => __DIR__ . '/../..' . '/application/themehandler.class.inc.php',
+        'ThemeHandlerService' => __DIR__ . '/../..' . '/application/themehandlerservice.class.inc.php',
         'ToolsLog' => __DIR__ . '/../..' . '/core/log.class.inc.php',
         'Trigger' => __DIR__ . '/../..' . '/core/trigger.class.inc.php',
         'TriggerOnObject' => __DIR__ . '/../..' . '/core/trigger.class.inc.php',

setup/compiler.class.inc.php

@@ -53,6 +53,8 @@ class MFCompiler
 {
 	const DATA_PRECOMPILED_FOLDER = 'data' . DIRECTORY_SEPARATOR . 'precompiled_styles' . DIRECTORY_SEPARATOR;
 
+	private static $oThemeHandlerService;
+
 	/** @var \ModelFactory */
 	protected $oFactory;
 
@@ -2792,11 +2794,10 @@ EOF;
 	/**
 	 * @param \MFElement $oBrandingNode
 	 * @param string $sTempTargetDir
-	 * @param string $sFinalTargetDir
 	 *
 	 * @throws \Exception
 	 */
-	protected function CompileThemes($oBrandingNode, $sTempTargetDir, $sFinalTargetDir)
+	protected function CompileThemes($oBrandingNode, $sTempTargetDir)
 	{
 		// Make sure temp. target dir. ends with a '/'
 		$sTempTargetDir .= '/';
@@ -2830,7 +2831,6 @@ EOF;
 				'imports_variable' => array(),
 				'imports_utility' => array(),
 				'stylesheets' => array(),
-				'precompiled_stylesheet' => '',
 			);
 
 			/** @var \DOMNodeList $oVariables */
@@ -2846,11 +2846,11 @@ EOF;
 			foreach($oImports as $oImport)
 			{
 				$sImportId = $oImport->getAttribute('id');
-				if($oImport->getAttribute('xsi:type') === 'variable')
+				if($oImport->getAttribute('xsi:type') === 'variables')
 				{
 					$aThemeParameters['imports_variable'][$sImportId] = $oImport->GetText();
 				}
-				else if($oImport->getAttribute('xsi:type') === 'utility')
+				else
 				{
 					$aThemeParameters['imports_utility'][$sImportId] = $oImport->GetText();
 				}
@@ -2863,8 +2863,11 @@ EOF;
 				$sStylesheetId = $oStylesheet->getAttribute('id');
 				$aThemeParameters['stylesheets'][$sStylesheetId] = $oStylesheet->GetText();
 			}
-			$aThemeParameters['precompiled_stylesheet'] = $oTheme->GetChildText('precompiled_stylesheet', '');
-			$aThemes[$sThemeId] = $aThemeParameters;
+
+			$aThemes[$sThemeId] = [
+				'theme_parameters' => $aThemeParameters,
+				'precompiled_stylesheet' => $oTheme->GetChildText('precompiled_stylesheet', '')
+			];
 		}
 
 		// Force to have a default theme if none in the DM
@@ -2881,8 +2884,11 @@ EOF;
 
 		// Compile themes
 		$fStart = microtime(true);
-		foreach($aThemes as $sThemeId => $aThemeParameters)
+		foreach($aThemes as $sThemeId => $aThemeInfos)
 		{
+			$aThemeParameters = $aThemeInfos['theme_parameters'];
+			$sPrecompiledStylesheet = $aThemeInfos['precompiled_stylesheet'];
+
 			$sThemeDir = $sThemesDir.$sThemeId;
 			if(!is_dir($sThemeDir))
 			{
@@ -2892,16 +2898,19 @@ EOF;
 			// Check if a precompiled version of the theme is supplied
 			$sPostCompilationLatestPrecompiledFile = $sPostCompilationPrecompiledThemeFolder . $sThemeId . ".css";
 
-			$sPrecompiledFileToUse = $this->UseLatestPrecompiledFile($sTempTargetDir, $aThemeParameters['precompiled_stylesheet'], $sPostCompilationLatestPrecompiledFile, $sThemeId);
+			$sPrecompiledFileToUse = $this->UseLatestPrecompiledFile($sTempTargetDir, $sPrecompiledStylesheet, $sPostCompilationLatestPrecompiledFile, $sThemeId);
 			if ($sPrecompiledFileToUse != null){
 				copy($sPrecompiledFileToUse, $sThemeDir.'/main.css');
 				// Make sure that the copy of the precompiled file is older than any other files to force a validation of the signature
 				touch($sThemeDir.'/main.css', 1577836800 /* 2020-01-01 00:00:00 */);
 			}
 
-			$bHasCompiled = ThemeHandler::CompileTheme($sThemeId, true, $this->sCompilationTimeStamp, $aThemeParameters, $aImportsPaths, $sTempTargetDir);
-			if ($bHasCompiled)
-			{
+			if (!static::$oThemeHandlerService) {
+				static::$oThemeHandlerService = new ThemeHandlerService();
+			}
+			$bHasCompiled = static::$oThemeHandlerService->CompileTheme($sThemeId, true, $this->sCompilationTimeStamp, $aThemeParameters, $aImportsPaths, $sTempTargetDir);
+
+			if ($bHasCompiled) {
 				SetupLog::Info("Replacing theme '$sThemeId' precompiled file in file $sPostCompilationLatestPrecompiledFile for next setup.");
 				copy($sThemeDir.'/main.css', $sPostCompilationLatestPrecompiledFile);
 			}else {
@@ -2911,6 +2920,10 @@ EOF;
 		$this->Log(sprintf('Themes compilation took: %.3f ms for %d themes.', (microtime(true) - $fStart)*1000.0, count($aThemes)));
 	}
 
+	public static function setThemeHandlerService(ThemeHandlerService $oThemeHandlerService): void {
+		self::$oThemeHandlerService = $oThemeHandlerService;
+	}
+
 	/**
 	 * Choose between precompiled files declared in datamodel XMLs or latest precompiled files generated after latest setup.
 	 *
@@ -3003,7 +3016,7 @@ EOF;
 			}
 			
 			// Compile themes 
-			$this->CompileThemes($oBrandingNode, $sTempTargetDir, $sFinalTargetDir);
+			$this->CompileThemes($oBrandingNode, $sTempTargetDir);
 		}
 	}
 

test/ItopTestCase.php

@@ -119,4 +119,75 @@ class ItopTestCase extends TestCase
 
 		return $method->invokeArgs($oObject, $aArgs);
 	}
+
+	public function RecurseRmdir($dir) {
+		if (is_dir($dir)) {
+			$objects = scandir($dir);
+			foreach ($objects as $object) {
+				if ($object != "." && $object != "..") {
+					if (is_dir($dir.DIRECTORY_SEPARATOR.$object))
+						$this->RecurseRmdir($dir.DIRECTORY_SEPARATOR.$object);
+					else
+						unlink($dir.DIRECTORY_SEPARATOR.$object);
+				}
+			}
+			rmdir($dir);
+		}
+	}
+
+	public function CreateTmpdir() {
+		$sTmpDir=tempnam(sys_get_temp_dir(),'');
+		if (file_exists($sTmpDir))
+		{
+			unlink($sTmpDir);
+		}
+		mkdir($sTmpDir);
+		if (is_dir($sTmpDir))
+		{
+			return $sTmpDir;
+		}
+
+		return sys_get_temp_dir();
+	}
+
+	public function RecurseMkdir($sDir){
+		if (strpos($sDir, DIRECTORY_SEPARATOR) === 0){
+			$sPath = DIRECTORY_SEPARATOR;
+		} else {
+			$sPath = "";
+		}
+
+		foreach (explode(DIRECTORY_SEPARATOR, $sDir) as $sSubDir){
+			if (($sSubDir === '..')) {
+				break;
+			}
+
+			if (( trim($sSubDir) === '' ) || ( $sSubDir === '.' )) {
+				continue;
+			}
+
+			$sPath .= $sSubDir . DIRECTORY_SEPARATOR;
+			if (!is_dir($sPath)) {
+				var_dump($sPath);
+				@mkdir($sPath);
+			}
+		}
+
+	}
+
+	public function RecurseCopy($src,$dst) {
+		$dir = opendir($src);
+		@mkdir($dst);
+		while(false !== ( $file = readdir($dir)) ) {
+			if (( $file != '.' ) && ( $file != '..' )) {
+				if ( is_dir($src . '/' . $file) ) {
+					$this->RecurseCopy($src . DIRECTORY_SEPARATOR . $file,$dst . DIRECTORY_SEPARATOR . $file);
+				}
+				else {
+					copy($src . DIRECTORY_SEPARATOR . $file,$dst . DIRECTORY_SEPARATOR . $file);
+				}
+			}
+		}
+		closedir($dir);
+	}
 }

test/application/ThemeHandlerTest.php

@@ -27,13 +27,13 @@ class ThemeHandlerTest extends ItopTestCase
 		$this->oCompileCSSServiceMock = $this->createMock('CompileCSSService');
 		ThemeHandler::mockCompileCSSService($this->oCompileCSSServiceMock);
 
-		$this->sTmpDir = $this->tmpdir();
+		$this->sTmpDir = $this->CreateTmpdir();
 		$this->aDirsToCleanup[] = $this->sTmpDir;
 
 		$this->recurseMkdir($this->sTmpDir."/branding/themes/basque-red");
 		$this->sCssPath = $this->sTmpDir.'/branding/themes/basque-red/main.css';
 		$this->sJsonThemeParamFile = $this->sTmpDir.'/branding/themes/basque-red/theme-parameters.json';
-		$this->recurse_copy(APPROOT."/test/application/theme-handler/expected/css", $this->sTmpDir."/branding/css");
+		$this->RecurseCopy(APPROOT."/test/application/theme-handler/expected/css", $this->sTmpDir."/branding/css");
 	}
 
 	public function tearDown()
@@ -43,56 +43,10 @@ class ThemeHandlerTest extends ItopTestCase
 		foreach ($this->aDirsToCleanup as $sDir)
 		{
 			echo $sDir;
-			$this->rrmdir($sDir);
+			$this->RecurseRmdir($sDir);
 		}
 	}
 
-	function rrmdir($dir) {
-		if (is_dir($dir)) {
-			$objects = scandir($dir);
-			foreach ($objects as $object) {
-				if ($object != "." && $object != "..") {
-					if (is_dir($dir."/".$object))
-						$this->rrmdir($dir."/".$object);
-					else
-						unlink($dir."/".$object);
-				}
-			}
-			rmdir($dir);
-		}
-	}
-
-	function tmpdir() {
-		$sTmpfile=tempnam(sys_get_temp_dir(),'');
-		if (file_exists($sTmpfile))
-		{
-			unlink($sTmpfile);
-		}
-		mkdir($sTmpfile);
-		if (is_dir($sTmpfile))
-		{
-			return $sTmpfile;
-		}
-
-		return sys_get_temp_dir();
-	}
-
-	public function recurse_copy($src,$dst) {
-		$dir = opendir($src);
-		@mkdir($dst);
-		while(false !== ( $file = readdir($dir)) ) {
-			if (( $file != '.' ) && ( $file != '..' )) {
-				if ( is_dir($src . '/' . $file) ) {
-					$this->recurse_copy($src . '/' . $file,$dst . '/' . $file);
-				}
-				else {
-					copy($src . '/' . $file,$dst . '/' . $file);
-				}
-			}
-		}
-		closedir($dir);
-	}
-
 	/**
 	 * Test used to be notified by CI when precompiled styles are not up to date anymore in code repository.
 	 *
@@ -151,8 +105,7 @@ class ThemeHandlerTest extends ItopTestCase
 						$aThemeParameters = [
 							'variables' => [],
 							'imports_utility' => [],
-							'stylesheets' => [],
-							'precompiled_stylesheet' => $sPrecompiledStylesheetUri,
+							'stylesheets' => []
 						];
 
 						/** @var \DOMNodeList $oVariables */
@@ -393,7 +346,7 @@ JSON;
 		$sAbsoluteImagePath = APPROOT.'test/application/theme-handler/copied/testimages/';
 		$this->recurseMkdir($sAbsoluteImagePath);
 		$this->aDirsToCleanup[] = dirname($sAbsoluteImagePath);
-		$this->recurse_copy(APPROOT.'test/application/theme-handler/expected/testimages/', $sAbsoluteImagePath);
+		$this->RecurseCopy(APPROOT.'test/application/theme-handler/expected/testimages/', $sAbsoluteImagePath);
 
 		//change approot-relative in css-variable to use absolute path
 		$sCssVarPath = $this->sTmpDir."/branding/css/DO_NOT_CHANGE.css-variables.scss";

test/setup/MFCompilerTest.php

@@ -18,12 +18,21 @@ class MFCompilerTest extends ItopTestCase {
 	/** @var \MFCompiler  */
 	private $oMFCompiler;
 
-	public function setUp()
-	{
+	private $sTmpDir;
+
+	public function setUp() {
 		parent::setUp();
 		require_once(APPROOT.'setup/compiler.class.inc.php');
+		require_once(APPROOT.'setup/modelfactory.class.inc.php');
+		require_once(__DIR__.'/SubMFCompiler.php');
 
-		$this->oMFCompiler = new MFCompiler($this->createMock(\ModelFactory::class), '');
+		$this->sTmpDir = $this->CreateTmpdir();
+		$this->oMFCompiler = new SubMFCompiler($this->createMock(\ModelFactory::class), '');
+	}
+
+	public function tearDown() {
+		parent::tearDown();
+		$this->RecurseRmdir($this->sTmpDir);
 	}
 
 	public static function Init(){
@@ -133,5 +142,46 @@ class MFCompilerTest extends ItopTestCase {
 		];
 	}
 
+	public function testCompileThemes(){
+		$sXmlDataCustoFilePath = realpath(__DIR__ . '/ressources/datamodels/datamodel-branding.xml');
+		$oDom = new MFDocument();
+		$oDom->load($sXmlDataCustoFilePath);
 
+		/** @var \MFElement $oBrandingNode */
+		$oBrandingNode = $oDom->GetNodes('branding')->item(0);
+
+		$this->RecurseMkdir($this->sTmpDir.'/branding/themes/fullmoon/');
+		file_put_contents($this->sTmpDir.'/branding/themes/fullmoon/main.css', "");
+
+		$aImportsPaths = array(
+			APPROOT.'css/',
+			APPROOT.'css/backoffice/main.scss',
+			$this->sTmpDir.'//',
+		);
+
+
+		$aThemeParameters = [
+			'variables' => [
+				'ibo-page-banner--background-color' => '$ibo-color-red-600',
+				'ibo-page-banner--text-color' => '$ibo-color-red-100',
+				'ibo-page-banner--text-content' => '"THIS IS A TEST INSTANCE"',
+			],
+			'imports_variable' => [ 'style2' => 'style2.scss'],
+			'imports_utility' => [ 'style1' => 'style1.scss', 'style3' => 'style3.scss'],
+			'stylesheets' => [
+				"fullmoon" => '../css/backoffice/main.scss',
+                "environment-banner" => '../css/backoffice/themes/page-banner.scss'
+			],
+		];
+
+		$oThemeHandlerService = $this->createMock(\ThemeHandlerService::class);
+		$oThemeHandlerService->expects($this->exactly(1))
+			->method("CompileTheme")
+			->with("fullmoon", true, $this->oMFCompiler->GetCompilationTimeStamp(), $aThemeParameters, $aImportsPaths, $this->sTmpDir . '/');
+
+
+		//CompileTheme($sThemeId, $bSetup = false, $sSetupCompilationTimestamp="", $aThemeParameters = null, $aImportsPaths = null, $sWorkingPath = null)
+		MFCompiler::setThemeHandlerService($oThemeHandlerService);
+		$this->oMFCompiler->CompileThemes($oBrandingNode, $this->sTmpDir);
+	}
 }

test/setup/SubMFCompiler.php

@@ -0,0 +1,14 @@
+<?php
+
+/**
+ * Class SubMFCompiler: used to call a protected method for testing purpose
+ */
+class SubMFCompiler extends MFCompiler {
+	public function CompileThemes($oBrandingNode, $sTempTargetDir) {
+		return parent::CompileThemes($oBrandingNode, $sTempTargetDir);
+	}
+
+	public function GetCompilationTimeStamp(){
+		return $this->sCompilationTimeStamp;
+	}
+}

test/setup/ressources/datamodels/datamodel-branding.xml

@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<itop_design xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" version="3.0">
+  <branding>
+    <themes>
+      <theme id="fullmoon" _delta="define">
+        <variables>
+          <variable id="ibo-page-banner--background-color">$ibo-color-red-600</variable>
+          <variable id="ibo-page-banner--text-color">$ibo-color-red-100</variable>
+          <variable id="ibo-page-banner--text-content">"THIS IS A TEST INSTANCE"</variable>
+        </variables>
+        <imports>
+          <import id="style1">style1.scss</import>
+          <import id="style2" xsi:type="variables">style2.scss</import>
+          <import id="style3" xsi:type="utility">style3.scss</import>
+        </imports>
+        <stylesheets>
+          <stylesheet id="fullmoon">../css/backoffice/main.scss</stylesheet>
+          <stylesheet id="environment-banner">../css/backoffice/themes/page-banner.scss</stylesheet>
+        </stylesheets>
+        <precompiled_stylesheet>itop-structure/precompiled-themes/test-red/main.css</precompiled_stylesheet>
+      </theme>
+    </themes>
+  </branding>
+</itop_design>