Skip to content

Commit

Permalink
Fixed issue: custom css/js not loaded on extended themes
Browse files Browse the repository at this point in the history
  • Loading branch information
LouisGac committed Feb 9, 2018
1 parent e600c10 commit bcda36f
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 2 deletions.
8 changes: 6 additions & 2 deletions application/controllers/admin/themes.php
Expand Up @@ -637,7 +637,6 @@ public function templatesavechanges()
$this->getController()->redirect(array("admin/themes/sa/upload"));
}


//$savefilename = $oEditedTemplate
if (!file_exists($oEditedTemplate->path.$relativePathEditfile) && !file_exists($oEditedTemplate->viewPath.$relativePathEditfile)) {
$oEditedTemplate->extendsFile($relativePathEditfile);
Expand All @@ -659,6 +658,11 @@ public function templatesavechanges()

$oEditedTemplate->actualizeLastUpdate();

// If the file is an asset file, we refresh asset number
if (in_array($relativePathEditfile, $cssfiles) || in_array($relativePathEditfile, $jsfiles)){
SettingGlobal::increaseAssetsversionnumber();
}

fclose($handle);
} else {
Yii::app()->user->setFlash('error', "The file $savefilename is not writable");
Expand All @@ -671,7 +675,7 @@ public function templatesavechanges()
Yii::app()->setFlashMessage(gT("We are sorry but you don't have permissions to do this."), 'error');
}

$this->getController()->redirect(array('admin/themes/', 'sa'=>'view', 'editfile'=>$relativePathEditfile, 'screenname'=>$screenname, 'templatename'=>$sTemplateName));
$this->getController()->redirect(array('admin/themes/', 'sa'=>'view', 'editfile'=>$relativePathEditfile, 'screenname'=>$screenname, 'templatename'=>$sTemplateName), true );
}

/**
Expand Down
43 changes: 43 additions & 0 deletions application/models/SettingGlobal.php
Expand Up @@ -70,4 +70,47 @@ public function updateSetting($settingname, $settingvalue)
}

}


/**
* Increase the asset version number in version.php
* This will force the refresh of the assets folders content
*/
static public function increaseAssetsversionnumber()
{
@ini_set('auto_detect_line_endings', true);
$sRootdir = Yii::app()->getConfig("rootdir");
$versionlines = file($sRootdir.DIRECTORY_SEPARATOR.'application'.DIRECTORY_SEPARATOR.'config'.DIRECTORY_SEPARATOR.'version.php');
$handle = fopen($sRootdir.DIRECTORY_SEPARATOR.'application'.DIRECTORY_SEPARATOR.'config'.DIRECTORY_SEPARATOR.'version.php', "w");
$iAssetNumber = self::generateAssetVersionNumber(Yii::app()->getConfig("assetsversionnumber"));

foreach ($versionlines as $line) {
if (strpos($line, 'assetsversionnumber') !== false) {
$line = '$config[\'assetsversionnumber\'] = '.$iAssetNumber.';'."\r\n";
}
fwrite($handle, $line);
}
fclose($handle);
Yii::app()->setConfig("assetsversionnumber", $iAssetNumber);
return;
}

/**
* with comfortUpate, we increase the asset number by one.
* so to be sure that the asset number from comfortUpdate will be different from the one generated here, we index it by 100000
*
* @param int $iAssetNumber the current asset number
* @return int the new asset number
*/
static public function generateAssetVersionNumber($iAssetNumber)
{
while ( $iAssetNumber == Yii::app()->getConfig("assetsversionnumber")) {
if ($iAssetNumber > 100000){
$iAssetNumber++;
}else{
$iAssetNumber = Yii::app()->getConfig("assetsversionnumber") + 100000;
}
}
return $iAssetNumber;
}
}

14 comments on commit bcda36f

@Shnoulle
Copy link
Collaborator

@Shnoulle Shnoulle commented on bcda36f Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? Then : version.php must go out of repo ?
Why not use a DB settings for this ?

PS : for regenerate assets in plugin i use a extra file : 1, 2, 3 … or a touch assets/ok unlink asstets/ok with a test of DB version vs Class constant.

@LouisGac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we removed the "touch" way of doing because directory modification date is not reliable enough.

protected function generatePath($file, $hashByName = false)

test DB vs "constant" is exactly what we're doing (the system is the very same as for DB upgrade)

@Shnoulle
Copy link
Collaborator

@Shnoulle Shnoulle commented on bcda36f Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test DB vs "constant" is exactly what we're doing (the system is the very same as for DB upgrade)

PHP file update version.php here fwrite($handle, $line);

DB upgrade didn't update version.php file, only DB

And about touch : you trye to use touch on directory, not add/remove a file on directory. Tested on a lot of different system.

@LouisGac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/LimeSurvey/LimeSurvey/blob/master/application/config/version.php#L16

When running comfort update, version.php is updated, then dbversion in db and in version are different, then the update of db is launched.

I agree I could make it more complex for asset version number, using a version number from the Theme XML itself

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know how DB update work … but if you update a PHP file in a directory , this file can't be included in GIT …

After this commit : application/config/version.php must be added to .gitignore

@LouisGac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

@LouisGac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because i can have a different buildnumber than yours in the same GIT commit id.

What happen if you update (generateAssetVersionNumber) 5 times and me only 3 times ?

@LouisGac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now: custom asset version number is > 100000, whereas repo asset version number is > 30000
So let say you start with version number 30014, if you update one time you'll be at 130014, two time you'll be at 130015, etc. If then you update using CU or manual update, then you'll be at 30015.

Again, I agree that a system based on the theme manifest would be better.

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is not here … i can have a different php file with same git commit id than you : then : the file must be in .gitignore

@LouisGac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

" i can have a different php file with same git commit id than you "
then pull, merge, problem solved...
or can you give an example where it will be a problem?

@LouisGac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but sure: not adding version.php at each commit can be annoying, I agree.
making the system more granular by using the Theme manifest would take time.
What we could do, if you insist, is to quickly add a file out of the repo containing a custom asset version to generate the asset path.

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is more : why not use only DB ?
After ComfortUpdate : clean assets and add 1
After manual update : inform user on manual
After Git update : git user know :)

Another solution : create a asset-version.php file with ONLY asset number (and goes out of .gitconfig like config.php)

@LouisGac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to keep a trace on it, the system now use a DB value as you asked:
3e746e1

Please sign in to comment.