Merge pull request #962 from cebe/700-fix-js-asset-position-conflict

fix js asset position conflict

framework/yii/base/View.php

@@ -142,11 +142,11 @@ class View extends Component
 	 */
 	public $dynamicPlaceholders = array();
 	/**
-	 * @var array list of the registered asset bundles. The keys are the bundle names, and the values
+	 * @var AssetBundle[] list of the registered asset bundles. The keys are the bundle names, and the values
-	 * are booleans indicating whether the bundles have been registered.
+	 * are the registered [[AssetBundle]] objects.
 	 * @see registerAssetBundle
 	 */
-	public $assetBundles;
+	public $assetBundles = array();
 	/**
 	 * @var string the page title
 	 */
@@ -523,6 +523,9 @@ class View extends Component
 		$this->trigger(self::EVENT_END_PAGE);
 
 		$content = ob_get_clean();
+		foreach(array_keys($this->assetBundles) as $bundle) {
+			$this->registerAssetFiles($bundle);
+		}
 		echo strtr($content, array(
 			self::PH_HEAD => $this->renderHeadHtml(),
 			self::PH_BODY_BEGIN => $this->renderBodyBeginHtml(),
@@ -530,7 +533,6 @@ class View extends Component
 		));
 
 		unset(
-			$this->assetBundles,
 			$this->metaTags,
 			$this->linkTags,
 			$this->css,
@@ -541,6 +543,24 @@ class View extends Component
 	}
 
 	/**
+	 * Registers all files provided by an asset bundle including depending bundles files.
+	 * Removes a bundle from [[assetBundles]] once registered.
+	 * @param string $name name of the bundle to register
+	 */
+	private function registerAssetFiles($name)
+	{
+		if (!isset($this->assetBundles[$name])) {
+			return;
+		}
+		$bundle = $this->assetBundles[$name];
+		foreach($bundle->depends as $dep) {
+			$this->registerAssetFiles($dep);
+		}
+		$bundle->registerAssets($this);
+		unset($this->assetBundles[$name]);
+	}
+
+	/**
 	 * Marks the beginning of an HTML body section.
 	 */
 	public function beginBody()
@@ -570,21 +590,44 @@ class View extends Component
 	 * Registers the named asset bundle.
 	 * All dependent asset bundles will be registered.
 	 * @param string $name the name of the asset bundle.
+	 * @param integer|null $position if set, this forces a minimum position for javascript files.
+	 * This will adjust depending assets javascript file position or fail if requirement can not be met.
+	 * If this is null, asset bundles position settings will not be changed.
+	 * See [[registerJsFile]] for more details on javascript position.
 	 * @return AssetBundle the registered asset bundle instance
 	 * @throws InvalidConfigException if the asset bundle does not exist or a circular dependency is detected
 	 */
-	public function registerAssetBundle($name)
+	public function registerAssetBundle($name, $position = null)
 	{
 		if (!isset($this->assetBundles[$name])) {
 			$am = $this->getAssetManager();
 			$bundle = $am->getBundle($name);
 			$this->assetBundles[$name] = false;
-			$bundle->registerAssets($this);
+			// register dependencies
+			$pos = isset($bundle->jsOptions['position']) ? $bundle->jsOptions['position'] : null;
+			foreach ($bundle->depends as $dep) {
+				$this->registerAssetBundle($dep, $pos);
+			}
 			$this->assetBundles[$name] = $bundle;
 		} elseif ($this->assetBundles[$name] === false) {
 			throw new InvalidConfigException("A circular dependency is detected for bundle '$name'.");
+		} else {
+			$bundle = $this->assetBundles[$name];
+		}
+
+		if ($position !== null) {
+			$pos = isset($bundle->jsOptions['position']) ? $bundle->jsOptions['position'] : null;
+			if ($pos === null) {
+				$bundle->jsOptions['position'] = $pos = $position;
+			} elseif ($pos > $position) {
+				throw new InvalidConfigException("An asset bundle that depends on '$name' has a higher javascript file position configured than '$name'.");
+			}
+			// update position for all dependencies
+			foreach ($bundle->depends as $dep) {
+				$this->registerAssetBundle($dep, $pos);
+			}
 		}
-		return $this->assetBundles[$name];
+		return $bundle;
 	}
 
 	/**

framework/yii/web/AssetBundle.php

@@ -130,7 +130,6 @@ class AssetBundle extends Object
 
 	/**
 	 * Registers the CSS and JS files with the given view.
-	 * This method will first register all dependent asset bundles.
 	 * It will then try to convert non-CSS or JS files (e.g. LESS, Sass) into the corresponding
 	 * CSS or JS files using [[AssetManager::converter|asset converter]].
 	 * @param \yii\base\View $view the view that the asset files to be registered with.
@@ -139,10 +138,6 @@ class AssetBundle extends Object
 	 */
 	public function registerAssets($view)
 	{
-		foreach ($this->depends as $name) {
-			$view->registerAssetBundle($name);
-		}
-
 		$this->publish($view->getAssetManager());
 
 		foreach ($this->js as $js) {

tests/unit/data/views/layout.php

@@ -0,0 +1,22 @@
+<?php
+/**
+ * @var $this \yii\base\View
+ * @var $content string
+ */
+?>
+<?php $this->beginPage(); ?>
+<!DOCTYPE html>
+<html>
+<head>
+	<title>Test</title>
+	<?php $this->head(); ?>
+</head>
+<body>
+<?php $this->beginBody(); ?>
+
+<?php echo $content; ?>
+
+<?php $this->endBody(); ?>
+</body>
+</html>
+<?php $this->endPage(); ?>

tests/unit/data/views/rawlayout.php

@@ -0,0 +1,5 @@
+<?php
+/**
+ * @var $this \yii\base\View
+ */
+?><?php $this->beginPage(); ?>1<?php $this->head(); ?>2<?php $this->beginBody(); ?>3<?php $this->endBody(); ?>4<?php $this->endPage(); ?>

tests/unit/data/views/simple.php

@@ -0,0 +1 @@
+This is a damn simple view file.

tests/unit/data/web/assets/.gitignore

@@ -0,0 +1,2 @@
+*
+!.gitignore

tests/unit/framework/web/AssetBundleTest.php

@@ -0,0 +1,264 @@
+<?php
+/**
+ * 
+ * 
+ * @author Carsten Brandt <mail@cebe.cc>
+ */
+
+namespace yiiunit\framework\web;
+
+use Yii;
+use yii\base\View;
+use yii\web\AssetBundle;
+use yii\web\AssetManager;
+
+/**
+ * @group web
+ */
+class AssetBundleTest extends \yiiunit\TestCase
+{
+	protected function setUp()
+	{
+		parent::setUp();
+		$this->mockApplication();
+
+		Yii::setAlias('@testWeb', '/');
+		Yii::setAlias('@testWebRoot', '@yiiunit/data/web');
+	}
+
+	protected function getView()
+	{
+		$view = new View();
+		$view->setAssetManager(new AssetManager(array(
+			'basePath' => '@testWebRoot/assets',
+			'baseUrl' => '@testWeb/assets',
+		)));
+
+		return $view;
+	}
+
+	public function testRegister()
+	{
+		$view = $this->getView();
+
+		$this->assertEmpty($view->assetBundles);
+		TestSimpleAsset::register($view);
+		$this->assertEquals(1, count($view->assetBundles));
+		$this->assertArrayHasKey('yiiunit\\framework\\web\\TestSimpleAsset', $view->assetBundles);
+		$this->assertTrue($view->assetBundles['yiiunit\\framework\\web\\TestSimpleAsset'] instanceof AssetBundle);
+
+		$expected = <<<EOF
+123<script src="/js/jquery.js"></script>
+4
+EOF;
+		$this->assertEquals($expected, $view->renderFile('@yiiunit/data/views/rawlayout.php'));
+	}
+
+	public function testSimpleDependency()
+	{
+		$view = $this->getView();
+
+		$this->assertEmpty($view->assetBundles);
+		TestAssetBundle::register($view);
+		$this->assertEquals(3, count($view->assetBundles));
+		$this->assertArrayHasKey('yiiunit\\framework\\web\\TestAssetBundle', $view->assetBundles);
+		$this->assertArrayHasKey('yiiunit\\framework\\web\\TestJqueryAsset', $view->assetBundles);
+		$this->assertArrayHasKey('yiiunit\\framework\\web\\TestAssetLevel3', $view->assetBundles);
+		$this->assertTrue($view->assetBundles['yiiunit\\framework\\web\\TestAssetBundle'] instanceof AssetBundle);
+		$this->assertTrue($view->assetBundles['yiiunit\\framework\\web\\TestJqueryAsset'] instanceof AssetBundle);
+		$this->assertTrue($view->assetBundles['yiiunit\\framework\\web\\TestAssetLevel3'] instanceof AssetBundle);
+
+		$expected = <<<EOF
+1<link href="/files/cssFile.css" rel="stylesheet">
+23<script src="/js/jquery.js"></script>
+<script src="/files/jsFile.js"></script>
+4
+EOF;
+		$this->assertEquals($expected, $view->renderFile('@yiiunit/data/views/rawlayout.php'));
+	}
+
+	public function positionProvider()
+	{
+		return array(
+			array(View::POS_HEAD, true),
+			array(View::POS_HEAD, false),
+			array(View::POS_BEGIN, true),
+			array(View::POS_BEGIN, false),
+			array(View::POS_END, true),
+			array(View::POS_END, false),
+		);
+	}
+
+	/**
+	 * @dataProvider positionProvider
+	 */
+	public function testPositionDependency($pos, $jqAlreadyRegistered)
+	{
+		$view = $this->getView();
+
+		$view->getAssetManager()->bundles['yiiunit\\framework\\web\\TestAssetBundle'] = array(
+			'jsOptions' => array(
+				'position' => $pos,
+			),
+		);
+
+		$this->assertEmpty($view->assetBundles);
+		if ($jqAlreadyRegistered) {
+			TestJqueryAsset::register($view);
+		}
+		TestAssetBundle::register($view);
+		$this->assertEquals(3, count($view->assetBundles));
+		$this->assertArrayHasKey('yiiunit\\framework\\web\\TestAssetBundle', $view->assetBundles);
+		$this->assertArrayHasKey('yiiunit\\framework\\web\\TestJqueryAsset', $view->assetBundles);
+		$this->assertArrayHasKey('yiiunit\\framework\\web\\TestAssetLevel3', $view->assetBundles);
+
+		$this->assertTrue($view->assetBundles['yiiunit\\framework\\web\\TestAssetBundle'] instanceof AssetBundle);
+		$this->assertTrue($view->assetBundles['yiiunit\\framework\\web\\TestJqueryAsset'] instanceof AssetBundle);
+		$this->assertTrue($view->assetBundles['yiiunit\\framework\\web\\TestAssetLevel3'] instanceof AssetBundle);
+
+		$this->assertArrayHasKey('position', $view->assetBundles['yiiunit\\framework\\web\\TestAssetBundle']->jsOptions);
+		$this->assertEquals($pos, $view->assetBundles['yiiunit\\framework\\web\\TestAssetBundle']->jsOptions['position']);
+		$this->assertArrayHasKey('position', $view->assetBundles['yiiunit\\framework\\web\\TestJqueryAsset']->jsOptions);
+		$this->assertEquals($pos, $view->assetBundles['yiiunit\\framework\\web\\TestJqueryAsset']->jsOptions['position']);
+		$this->assertArrayHasKey('position', $view->assetBundles['yiiunit\\framework\\web\\TestAssetLevel3']->jsOptions);
+		$this->assertEquals($pos, $view->assetBundles['yiiunit\\framework\\web\\TestAssetLevel3']->jsOptions['position']);
+
+		switch($pos)
+		{
+			case View::POS_HEAD:
+				$expected = <<<EOF
+1<link href="/files/cssFile.css" rel="stylesheet">
+<script src="/js/jquery.js"></script>
+<script src="/files/jsFile.js"></script>
+234
+EOF;
+			break;
+			case View::POS_BEGIN:
+				$expected = <<<EOF
+1<link href="/files/cssFile.css" rel="stylesheet">
+2<script src="/js/jquery.js"></script>
+<script src="/files/jsFile.js"></script>
+34
+EOF;
+			break;
+			default:
+			case View::POS_END:
+				$expected = <<<EOF
+1<link href="/files/cssFile.css" rel="stylesheet">
+23<script src="/js/jquery.js"></script>
+<script src="/files/jsFile.js"></script>
+4
+EOF;
+			break;
+		}
+		$this->assertEquals($expected, $view->renderFile('@yiiunit/data/views/rawlayout.php'));
+	}
+
+	public function positionProvider2()
+	{
+		return array(
+			array(View::POS_BEGIN, true),
+			array(View::POS_BEGIN, false),
+			array(View::POS_END, true),
+			array(View::POS_END, false),
+		);
+	}
+
+	/**
+	 * @dataProvider positionProvider
+	 */
+	public function testPositionDependencyConflict($pos, $jqAlreadyRegistered)
+	{
+		$view = $this->getView();
+
+		$view->getAssetManager()->bundles['yiiunit\\framework\\web\\TestAssetBundle'] = array(
+			'jsOptions' => array(
+				'position' => $pos - 1,
+			),
+		);
+		$view->getAssetManager()->bundles['yiiunit\\framework\\web\\TestJqueryAsset'] = array(
+			'jsOptions' => array(
+				'position' => $pos,
+			),
+		);
+
+		$this->assertEmpty($view->assetBundles);
+		if ($jqAlreadyRegistered) {
+			TestJqueryAsset::register($view);
+		}
+		$this->setExpectedException('yii\\base\\InvalidConfigException');
+		TestAssetBundle::register($view);
+	}
+
+	public function testCircularDependency()
+	{
+		$this->setExpectedException('yii\\base\\InvalidConfigException');
+		TestAssetCircleA::register($this->getView());
+	}
+}
+
+class TestSimpleAsset extends AssetBundle
+{
+	public $basePath = '@testWebRoot/js';
+	public $baseUrl = '@testWeb/js';
+	public $js = array(
+		'jquery.js',
+	);
+}
+
+class TestAssetBundle extends AssetBundle
+{
+	public $basePath = '@testWebRoot/files';
+	public $baseUrl = '@testWeb/files';
+	public $css = array(
+		'cssFile.css',
+	);
+	public $js = array(
+		'jsFile.js',
+	);
+	public $depends = array(
+		'yiiunit\\framework\\web\\TestJqueryAsset'
+	);
+}
+
+class TestJqueryAsset extends AssetBundle
+{
+	public $basePath = '@testWebRoot/js';
+	public $baseUrl = '@testWeb/js';
+	public $js = array(
+		'jquery.js',
+	);
+	public $depends = array(
+		'yiiunit\\framework\\web\\TestAssetLevel3'
+	);
+}
+
+class TestAssetLevel3 extends AssetBundle
+{
+	public $basePath = '@testWebRoot/js';
+	public $baseUrl = '@testWeb/js';
+}
+
+class TestAssetCircleA extends AssetBundle
+{
+	public $basePath = '@testWebRoot/js';
+	public $baseUrl = '@testWeb/js';
+	public $js = array(
+		'jquery.js',
+	);
+	public $depends = array(
+		'yiiunit\\framework\\web\\TestAssetCircleB'
+	);
+}
+
+class TestAssetCircleB extends AssetBundle
+{
+	public $basePath = '@testWebRoot/js';
+	public $baseUrl = '@testWeb/js';
+	public $js = array(
+		'jquery.js',
+	);
+	public $depends = array(
+		'yiiunit\\framework\\web\\TestAssetCircleA'
+	);
+}