Merge branch '3.1' into 3.2

Conflicts:
	dev/Debug.php
	docs/en/05_Contributing/01_Code.md
	forms/FormField.php
	i18n/i18nTextCollector.php
	model/DataQuery.php

.travis.yml

@@ -1,5 +1,11 @@
 language: php
 
+sudo: false
+
+cache:
+  directories:
+    - $HOME/.composer/cache
+
 php:
   - 5.4
 

_config/i18n.yml

@@ -13,3 +13,8 @@ Name: defaulti18n
 i18n:
   module_priority:
     - other_modules
+---
+Name: textcollector
+---
+Injector:
+  i18nTextCollector_Writer: 'i18nTextCollector_Writer_RailsYaml'

admin/code/CMSMenu.php

@@ -1,22 +1,22 @@
 <?php
 /**
- * The object manages the main CMS menu. See {@link LeftAndMain::init()} for
+ * The object manages the main CMS menu. See {@link LeftAndMain::init()} for 
  * example usage.
- *
- * The menu will be automatically populated with menu items for subclasses of
- * {@link LeftAndMain}. That is, for each class in the CMS that creates an
- * administration panel, a CMS menu item will be created. The default
- * configuration will also include a 'help' link to the SilverStripe user
+ * 
+ * The menu will be automatically populated with menu items for subclasses of 
+ * {@link LeftAndMain}. That is, for each class in the CMS that creates an 
+ * administration panel, a CMS menu item will be created. The default 
+ * configuration will also include a 'help' link to the SilverStripe user 
  * documentation.
  *
  * Additional CMSMenu items can be added through {@link LeftAndMainExtension::init()}
  * extensions added to {@link LeftAndMain}.
- *
+ * 
  * @package framework
  * @subpackage admin
  */
 class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
-
+	
 	/**
 	 * An array of changes to be made to the menu items, in the order that the changes should be
 	 * applied.  Each item is a map in one of the two forms:
@@ -24,7 +24,7 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 	 *  - array('type' => 'remove', 'code' => 'codename' )
 	 */
 	protected static $menu_item_changes = array();
-
+	
 	/**
 	 * Set to true if clear_menu() is called, to indicate that the default menu shouldn't be
 	 * included
@@ -32,7 +32,7 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 	protected static $menu_is_cleared = false;
 
 	/**
-	 * Generate CMS main menu items by collecting valid
+	 * Generate CMS main menu items by collecting valid 
 	 * subclasses of {@link LeftAndMain}
 	 */
 	public static function populate_menu() {
@@ -46,13 +46,13 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 	 * @return The result of the operation
 	 * @todo A director rule is added when a controller link is added, but it won't be removed
 	 *			when the item is removed. Functionality needed in {@link Director}.
-	 */
+	 */	
 	public static function add_controller($controllerClass) {
 		if($menuItem = self::menuitem_for_controller($controllerClass)) {
 			self::add_menu_item_obj($controllerClass, $menuItem);
 		}
 	}
-
+	
 	/**
 	 * Return a CMSMenuItem to add the given controller to the CMSMenu
 	 */
@@ -60,7 +60,7 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 		$urlBase      = Config::inst()->get($controllerClass, 'url_base', Config::FIRST_SET);
 		$urlSegment   = Config::inst()->get($controllerClass, 'url_segment', Config::FIRST_SET);
 		$menuPriority = Config::inst()->get($controllerClass, 'menu_priority', Config::FIRST_SET);
-
+		
 		// Don't add menu items defined the old way
 		if($urlSegment === null && $controllerClass != "CMSMain") return;
 
@@ -75,7 +75,7 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 		return new CMSMenuItem($menuTitle, $link, $controllerClass, $menuPriority);
 	}
 
-
+	
 	/**
 	 * Add an arbitrary URL to the CMS menu.
 	 *
@@ -91,7 +91,7 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 	public static function add_link($code, $menuTitle, $url, $priority = -1, $attributes = null) {
 		return self::add_menu_item($code, $menuTitle, $url, null, $priority, $attributes);
 	}
-
+	
 	/**
 	 * Add a navigation item to the main administration menu showing in the top bar.
 	 *
@@ -99,10 +99,10 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 	 *
 	 * @param string $code Unique identifier for this menu item (e.g. used by {@link replace_menu_item()} and
 	 * 					{@link remove_menu_item}. Also used as a CSS-class for icon customization.
-	 * @param string $menuTitle Localized title showing in the menu bar
+	 * @param string $menuTitle Localized title showing in the menu bar 
 	 * @param string $url A relative URL that will be linked in the menu bar.
-	 * @param string $controllerClass The controller class for this menu, used to check permisssions.
-	 * 					If blank, it's assumed that this is public, and always shown to users who
+	 * @param string $controllerClass The controller class for this menu, used to check permisssions.  
+	 * 					If blank, it's assumed that this is public, and always shown to users who 
 	 * 					have the rights to access some other part of the admin area.
 	 * @param array $attributes an array of attributes to include on the link.
 	 *
@@ -114,10 +114,10 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 		if($controllerClass) {
 			$code = $controllerClass;
 		}
-
+		
 		return self::replace_menu_item($code, $menuTitle, $url, $controllerClass, $priority, $attributes);
 	}
-
+	
 	/**
 	 * Get a single menu item by its code value.
 	 *
@@ -126,9 +126,9 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 	 */
 	public static function get_menu_item($code) {
 		$menuItems = self::get_menu_items();
-		return (isset($menuItems[$code])) ? $menuItems[$code] : false;
+		return (isset($menuItems[$code])) ? $menuItems[$code] : false; 
 	}
-
+	
 	/**
 	 * Get all menu entries.
 	 *
@@ -142,26 +142,26 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 			$cmsClasses = self::get_cms_classes();
 			foreach($cmsClasses as $cmsClass) {
 				$menuItem = self::menuitem_for_controller($cmsClass);
-				if($menuItem) $menuItems[$cmsClass] = $menuItem;
+				if($menuItem) $menuItems[Convert::raw2htmlname(str_replace('\\', '-', $cmsClass))] = $menuItem;
 			}
 		}
-
+		
 		// Apply changes
 		foreach(self::$menu_item_changes as $change) {
 			switch($change['type']) {
 				case 'add':
 					$menuItems[$change['code']] = $change['item'];
 					break;
-
+				
 				case 'remove':
 					unset($menuItems[$change['code']]);
 					break;
-
+					
 				default:
 					user_error("Bad menu item change type {$change[type]}", E_USER_WARNING);
 			}
 		}
-
+		
 		// Sort menu items according to priority, then title asc
 		$menuPriority = array();
 		$menuTitle    = array();
@@ -170,14 +170,14 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 			$menuTitle[$key]    = $menuItem->title;
 		}
 		array_multisort($menuPriority, SORT_DESC, $menuTitle, SORT_ASC, $menuItems);
-
+		
 		return $menuItems;
 	}
-
+	
 	/**
 	 * Get all menu items that the passed member can view.
 	 * Defaults to {@link Member::currentUser()}.
-	 *
+	 * 
 	 * @param Member $member
 	 * @return array
 	 */
@@ -185,7 +185,7 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 		if(!$member && $member !== FALSE) {
 			$member = Member::currentUser();
 		}
-
+		
 		$viewableMenuItems = array();
 		$allMenuItems = self::get_menu_items();
 		if($allMenuItems) foreach($allMenuItems as $code => $menuItem) {
@@ -200,13 +200,13 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 					if(!$controllerObj->canView($member)) continue;
 				}
 			}
-
+			
 			$viewableMenuItems[$code] = $menuItem;
 		}
-
+		
 		return $viewableMenuItems;
 	}
-
+	
 	/**
 	 * Removes an existing item from the menu.
 	 *
@@ -215,7 +215,7 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 	public static function remove_menu_item($code) {
 		self::$menu_item_changes[] = array('type' => 'remove', 'code' => $code);
 	}
-
+	
 	/**
 	 * Clears the entire menu
 	 */
@@ -229,11 +229,11 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 	 *
 	 * @param string $code Unique identifier for this menu item (e.g. used by {@link replace_menu_item()} and
 	 * 					{@link remove_menu_item}. Also used as a CSS-class for icon customization.
-	 * @param string $menuTitle Localized title showing in the menu bar
+	 * @param string $menuTitle Localized title showing in the menu bar 
 	 * @param string $url A relative URL that will be linked in the menu bar.
 	 * 					Make sure to add a matching route via {@link Director::$rules} to this url.
-	 * @param string $controllerClass The controller class for this menu, used to check permisssions.
-	 * 					If blank, it's assumed that this is public, and always shown to users who
+	 * @param string $controllerClass The controller class for this menu, used to check permisssions.  
+	 * 					If blank, it's assumed that this is public, and always shown to users who 
 	 * 					have the rights to access some other part of the admin area.
 	 * @param array $attributes an array of attributes to include on the link.
 	 *
@@ -253,7 +253,7 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 			'item' => $item,
 		);
 	}
-
+	
 	/**
 	 * Add a previously built menu item object to the menu
 	 */
@@ -292,10 +292,10 @@ class CMSMenu extends Object implements IteratorAggregate, i18nEntityProvider {
 				if(!$classReflection->isInstantiable()) unset($subClasses[$key]);
 			}
 		}
-
+		
 		return $subClasses;
 	}
-
+	
 	/**
 	 * IteratorAggregate Interface Method.  Iterates over the menu items.
 	 */

admin/code/LeftAndMain.php

@@ -555,7 +555,7 @@ class LeftAndMain extends Controller implements PermissionProvider {
 	public static function menu_icon_for_class($class) {
 		$icon = Config::inst()->get($class, 'menu_icon', Config::FIRST_SET);
 		if (!empty($icon)) {
-			$class = strtolower($class);
+			$class = strtolower(Convert::raw2htmlname(str_replace('\\', '-', $class)));
 			return ".icon.icon-16.icon-{$class} { background: url('{$icon}'); } ";
 		}
 		return '';

admin/javascript/LeftAndMain.Menu.js

@@ -176,7 +176,7 @@
 			updateMenuFromResponse: function(xhr) {
 				var controller = xhr.getResponseHeader('X-Controller');
 				if(controller) {
-					var item = this.find('li#Menu-' + controller);
+					var item = this.find('li#Menu-' + controller.replace(/\\/g, '-').replace(/[^a-zA-Z0-9\-_:.]+/, ''));
 					if(!item.hasClass('current')) item.select();
 				}
 				this.updateItems();

control/injector/Injector.php

@@ -272,7 +272,16 @@ class Injector {
 	 * @return Injector Reference to restored active Injector instance
 	 */
 	public static function unnest() {
-		return self::set_inst(self::$instance->nestedFrom);
+		if (self::inst()->nestedFrom) {
+			self::set_inst(self::inst()->nestedFrom);
+		}
+		else {
+			user_error(
+				"Unable to unnest root Injector, please make sure you don't have mis-matched nest/unnest",
+				E_USER_WARNING
+			);
+		}
+		return self::inst();
 	}
 
 	/**

core/Config.php

@@ -237,7 +237,16 @@ class Config {
 	 * @return Config Reference to new active Config instance
 	 */
 	public static function unnest() {
-		return self::set_instance(self::$instance->nestedFrom);
+		if (self::inst()->nestedFrom) {
+			self::set_instance(self::inst()->nestedFrom);
+		}
+		else {
+			user_error(
+				"Unable to unnest root Config, please make sure you don't have mis-matched nest/unnest",
+				E_USER_WARNING
+			);
+		}
+		return self::inst();
 	}
 
 	/**

core/Constants.php

@@ -185,9 +185,10 @@ if(!isset($_SERVER['HTTP_HOST'])) {
 	}
 }
 
-if (defined('SS_ALLOWED_HOSTS')) {
+// Filter by configured allowed hosts
+if (defined('SS_ALLOWED_HOSTS') && php_sapi_name() !== "cli") {
 	$all_allowed_hosts = explode(',', SS_ALLOWED_HOSTS);
-	if (!in_array($_SERVER['HTTP_HOST'], $all_allowed_hosts)) {
+	if (!isset($_SERVER['HTTP_HOST']) || !in_array($_SERVER['HTTP_HOST'], $all_allowed_hosts)) {
 		header('HTTP/1.1 400 Invalid Host', true, 400);
 		die();
 	}

dev/Debug.php

@@ -524,7 +524,8 @@ function exceptionHandler($exception) {
 	$file = $exception->getFile();
 	$line = $exception->getLine();
 	$context = $exception->getTrace();
-	return Debug::fatalHandler($errno, $message, $file, $line, $context);
+	Debug::fatalHandler($errno, $message, $file, $line, $context);
+	exit(1);
 }
 
 /**
@@ -558,6 +559,7 @@ function errorHandler($errno, $errstr, $errfile, $errline) {
 		case E_CORE_ERROR:
 		case E_USER_ERROR:
 		default:
-			return Debug::fatalHandler($errno, $errstr, $errfile, $errline, debug_backtrace());
+			Debug::fatalHandler($errno, $errstr, $errfile, $errline, debug_backtrace());
+			exit(1);
 	}
 }

docs/en/00_Getting_Started/index.md

@@ -19,9 +19,9 @@ Other ways to get SilverStripe:
 To run SilverStripe on Linux/Unix, set up one of the following web servers: 
 
 *  [Install using Apache](installation) - our preferred platform
-*  [Install using Lighttpd](installation/how_to/configure_lighttpd) - fast, but a bit tricker to get going
+*  [Install using Lighttpd](installation/how_to/configure_lighttpd) - fast, but a bit trickier to get going
 *  [Install using Nginx](installation/how_to/configure_nginx) - Super fast at serving static files. Great for large traffic sites.
-*  [Install using nginx and HHVM](installation/how_to/setup_nginx_and_hhvm) - nginx and [HHVM](http://hhvm.com/) as a faster alternative to PHP
+*  [Install using nginx and HHVM](installation/how_to/setup_nginx_and_hhvm) - nginx and [HHVM](http://hhvm.com/) as a faster alternative to PHP.
 
 ### Windows
 

docs/en/01_Tutorials/index.md

@@ -27,6 +27,7 @@ These include video screencasts, written tutorials and code examples to get you
 * [Lesson 15: Building a Search Form](http://www.silverstripe.org/learn/lessons/building-a-search-form)
 * [Lesson 16: Lists and Pagination](http://www.silverstripe.org/learn/lessons/lists-and-pagination)
 * [Lesson 17: Ajax Behaviour and Viewable Data](http://www.silverstripe.org/learn/lessons/ajax-behaviour-and-viewabledata)
+* [Lesson 18: Dealing with Arbitrary Template Data](http://www.silverstripe.org/learn/lessons/dealing-with-arbitrary-template-data)
 
 ## Help: If you get stuck
 

docs/en/02_Developer_Guides/01_Templates/04_Rendering_Templates.md

@@ -80,7 +80,7 @@ does, such as `ArrayData` or `ArrayList`.
 					'Name' => 'John',
 					'Role' => 'Head Coach',
 					'Experience' => $experience
-				))->renderWith("AjaxTemplate");
+				)))->renderWith("AjaxTemplate");
 			} else {
 				return $this->httpError(404);
 			}

docs/en/02_Developer_Guides/05_Extending/04_Shortcodes.md

@@ -56,7 +56,7 @@ First we need to define a callback for the shortcode.
 			'MyShortCodeMethod' => 'HTMLText'
 		);
 
-		public function MyShortCodeMethod($arguments, $content = null, $parser = null, $tagName) {
+		public static function MyShortCodeMethod($arguments, $content = null, $parser = null, $tagName) {
 			return "<em>" . $tagName . "</em> " . $content . "; " . count($arguments) . " arguments.";
 		}
 	}

docs/en/02_Developer_Guides/08_Performance/01_Caching.md

@@ -117,14 +117,16 @@ To use this backend, you need a memcached daemon and the memcache PECL extension
 		'primary_memcached', 
 		'Memcached',
 		array(
-			'host' => 'localhost', 
-			'port' => 11211, 
-			'persistent' => true, 
-			'weight' => 1, 
-			'timeout' => 5,
-			'retry_interval' => 15, 
-			'status' => true, 
-			'failure_callback' => '' 
+			'servers' => array(
+				'host' => 'localhost', 
+				'port' => 11211, 
+				'persistent' => true, 
+				'weight' => 1, 
+				'timeout' => 5,
+				'retry_interval' => 15, 
+				'status' => true, 
+				'failure_callback' => ''
+			)
 		)
 	);
 	SS_Cache::pick_backend('primary_memcached', 'any', 10);

docs/en/02_Developer_Guides/15_Customising_the_Admin_Interface/How_Tos/Customise_CMS_Menu.md

@@ -1,6 +1,6 @@
 # How to customize the CMS Menu
 
-## Adding a administration panel
+## Adding an administration panel
 
 Every time you add a new extension of the `[api:LeftAndMain]` class to the CMS,
 SilverStripe will automatically create a new `[api:CMSMenuItem]` for it

docs/en/05_Contributing/01_Code.md

@@ -1,6 +1,9 @@
+title: Contributing Code
+summary: Fix bugs and add new features to help make SilverStripe better.
+
 # Contributing Code - Submiting Bugfixes and Enhancements
 
-SilverStripe will never be finished, and we need your help to keep making it better.  If you're a developer a great way to get involved is to contribute patches to our modules and core codebase, fixing bugs or adding feautres.
+SilverStripe will never be finished, and we need your help to keep making it better.  If you're a developer a great way to get involved is to contribute patches to our modules and core codebase, fixing bugs or adding features.
 
 The SilverStripe core modules (`framework` and `cms`), as well as some of the more popular modules are in
 git version control. SilverStripe hosts its modules on [github.com/silverstripe](http://github.com/silverstripe) and [github.com/silverstripe-labs](http://github.com/silverstripe-labs).  After [installing git](http://help.github.com/git-installation-redirect) and creating a [free github.com account](https://github.com/signup/free), you can "fork" a module,
@@ -16,7 +19,9 @@ We ask for this so that the ownership in the license is clear and unambiguous, a
 
 ## Step-by-step: From forking to sending the pull request
 
-_**NOTE:** The commands on this page assume that you are targetting framework version 3.2
+<div class="notice" markdown='1'>
+**Note:** Please adjust the commands below to the version of SilverStripe that you're targeting.
+</div>
 
 1. Install the project through composer. The process is described in detail in "[Installation through Composer](../getting_started/composer#contributing)".
 
@@ -50,18 +55,18 @@ _**NOTE:** The commands on this page assume that you are targetting framework ve
 
 		# [make sure all your changes are committed as necessary in branch]
 		git fetch upstream
-		git rebase upstream/3.1
+		git rebase upstream/3.2
 
 6. When development is complete, [squash all commit related to a single issue into a single commit](code#squash-all-commits-related-to-a-single-issue-into-a-single-commit).
 
 		git fetch upstream
-		git rebase -i upstream/3.1
+		git rebase -i upstream/3.2
 
 7. Push release candidate branch to GitHub 
 
 		git push origin ###-description
 
-8. Issue pull request on GitHub.  Visit your forked respoistory on GitHub.com and click the "Create Pull Request" button nex tot the new branch.
+8. Issue pull request on GitHub.  Visit your forked repository on GitHub.com and click the "Create Pull Request" button next to the new branch.
 
 The core team is then responsible for reviewing patches and deciding if they will make it into core.  If
 there are any problems they will follow up with you, so please ensure they have a way to contact you!
@@ -79,11 +84,11 @@ A core committer will also "label" your PR using the labels defined in GitHub, t
 
 The current GitHub labels are grouped into 5 sections:
 
- 1. Changes - These are designed to signal what kind of change they are and how they fit into the [Semantic Versioning](http://semver.org/) schema
- 2. Impact - What impact does this bug/issue/fix have, does it break a feature completely, is it just a side effect or is it trivial and not a bit problem (but a bit annoying)
- 3. Effort - How much effort is required to fix this issue?
- 4. Type - What aspect of the system the PR/issue covers
- 5. Feedback - Are we waiting on feedback, if so who from? Typically used for issues that are likely to take a while to have feedback given
+1. *Changes* - These are designed to signal what kind of change they are and how they fit into the [Semantic Versioning](http://semver.org/) schema
+2. *Impact* - What impact does this bug/issue/fix have, does it break a feature completely, is it just a side effect or is it trivial and not a bit problem (but a bit annoying)
+3. *Effort* - How much effort is required to fix this issue?
+4. *Type* - What aspect of the system the PR/issue covers
+5. *Feedback* - Are we waiting on feedback, if so who from? Typically used for issues that are likely to take a while to have feedback given
 
 | Label | Purpose |
 | ----- | ------- |
@@ -102,9 +107,9 @@ The current GitHub labels are grouped into 5 sections:
 | feedback-required/core-team | Core team members need to give an in-depth consideration |
 | feedback-required/author | This issue is awaiting feedback from the original author of the PR |
 
-### Workflow Diagram ###
+### Workflow Diagram
 
-![Workflow diagram](http://www.silverstripe.org/assets/doc-silverstripe-org/collaboration-on-github.png)
+[![Workflow diagram](http://www.silverstripe.org/assets/doc-silverstripe-org/collaboration-on-github.png)](http://www.silverstripe.org/assets/doc-silverstripe-org/collaboration-on-github.png)
 
 ### Quickfire Do's and Don't's
 
@@ -112,18 +117,18 @@ If you aren't familiar with git and GitHub, try reading the ["GitHub bootcamp do
 We also found the [free online git book](http://git-scm.com/book/) and the [git crash course](http://gitref.org/) useful.
 If you're familiar with it, here's the short version of what you need to know. Once you fork and download the code:
 
-  *  **Don't develop on the master branch.** Always create a development branch specific to "the issue" you're working on (on our [GitHub repository's issues](https://github.com/silverstripe/silverstripe-framework/issues)). Name it by issue number and description. For example, if you're working on Issue #100, a `DataObject::get_one()` bugfix, your development branch should be called 100-dataobject-get-one. If you decide to work on another issue mid-stream, create a new branch for that issue--don't work on both in one branch.
+* **Don't develop on the master branch.** Always create a development branch specific to "the issue" you're working on (on our [GitHub repository's issues](https://github.com/silverstripe/silverstripe-framework/issues)). Name it by issue number and description. For example, if you're working on Issue #100, a `DataObject::get_one()` bugfix, your development branch should be called 100-dataobject-get-one. If you decide to work on another issue mid-stream, create a new branch for that issue--don't work on both in one branch.
 
-  * **Do not merge the upstream master** with your development branch; *rebase* your branch on top of the upstream master.
+* **Do not merge the upstream master** with your development branch; *rebase* your branch on top of the upstream master.
 
-  * **A single development branch should represent changes related to a single issue.** If you decide to work on another issue, create another branch.
+* **A single development branch should represent changes related to a single issue.** If you decide to work on another issue, create another branch.
 
-  * **Squash your commits, so that each commit addresses a single issue.** After you rebase your work on top of the upstream master, you can squash multiple commits into one. Say, for instance, you've got three commits in related to Issue #100. Squash all three into one with the message "Issue #100 Description of the issue here." We won't accept pull requests for multiple commits related to a single issue; it's up to you to squash and clean your commit tree. (Remember, if you squash commits you've already pushed to GitHub, you won't be able to push that same branch again. Create a new local branch, squash, and push the new squashed branch.)
+* **Squash your commits, so that each commit addresses a single issue.** After you rebase your work on top of the upstream master, you can squash multiple commits into one. Say, for instance, you've got three commits in related to Issue #100. Squash all three into one with the message "Description of the issue here (fixes #100)" We won't accept pull requests for multiple commits related to a single issue; it's up to you to squash and clean your commit tree. (Remember, if you squash commits you've already pushed to GitHub, you won't be able to push that same branch again. Create a new local branch, squash, and push the new squashed branch.)
 
-  * **Choose the correct branch**: Assume the current release is 3.0.3, and 3.1.0 is in beta state.
-  Most pull requests should go against the `3.1.x-dev` *pre-release branch*, only critical bugfixes
-  against the `3.0.x-dev` *release branch*. If you're changing an API or introducing a major feature,
-  the pull request should go against `master` (read more about our [release process](release_process)). Branches are periodically merged "upwards" (3.0 into 3.1, 3.1 into master).
+* **Choose the correct branch**: Assume the current release is 3.0.3, and 3.1.0 is in beta state.
+Most pull requests should go against the `3.1.x-dev` *pre-release branch*, only critical bugfixes
+against the `3.0.x-dev` *release branch*. If you're changing an API or introducing a major feature,
+the pull request should go against `master` (read more about our [release process](release_process)). Branches are periodically merged "upwards" (3.0 into 3.1, 3.1 into master).
 
 ### Editing files directly on GitHub.com
 
@@ -133,17 +138,16 @@ After you have edited the file, GitHub will offer to create a pull request for y
 
 ## Check List
 
-*  Adhere to our [coding conventions](/getting_started/coding_conventions)
-*  If your patch is extensive, discuss it first on the [silverstripe-dev google group](https://groups.google.com/group/silverstripe-dev) (ideally before doing any serious coding)
-*  When working on existing tickets, provide status updates through ticket comments
-*  Check your patches against the "master" branch, as well as the latest release branch
-*  Write [unit tests](../developer_guides/testing/unit_testing)
-*  Write [Behat integration tests](https://github.com/silverstripe-labs/silverstripe-behat-extension) for any interface changes
-*  Describe specifics on how to test the effects of the patch
-*  It's better to submit multiple patches with separate bits of functionality than a big patch containing lots of
-changes
-*  Only submit a pull request for work you expect to be ready to merge. Work in progress is best discussed in an issue, or on your own repository fork.
-*  Document your code inline through [PHPDoc](http://en.wikipedia.org/wiki/PHPDoc) syntax. See our 
+* Adhere to our [coding conventions](/getting_started/coding_conventions)
+* If your patch is extensive, discuss it first on the [silverstripe-dev google group](https://groups.google.com/group/silverstripe-dev) (ideally before doing any serious coding)
+* When working on existing tickets, provide status updates through ticket comments
+* Check your patches against the "master" branch, as well as the latest release branch
+* Write [unit tests](../developer_guides/testing/unit_testing)
+* Write [Behat integration tests](https://github.com/silverstripe-labs/silverstripe-behat-extension) for any interface changes
+* Describe specifics on how to test the effects of the patch
+* It's better to submit multiple patches with separate bits of functionality than a big patch containing lots of changes
+* Only submit a pull request for work you expect to be ready to merge. Work in progress is best discussed in an issue, or on your own repository fork.
+* Document your code inline through [PHPDoc](http://en.wikipedia.org/wiki/PHPDoc) syntax. See our 
 [API documentation](http://api.silverstripe.org/3.1/) for good examples.
 * Check and update documentation on [docs.silverstripe.org](http://docs.silverstripe.org). Check for any references to functionality deprecated or extended through your patch. Documentation changes should be included in the patch.
 * If you get stuck, please post to the [forum](http://silverstripe.org/forum) or for deeper core problems, to the [core mailinglist](https://groups.google.com/forum/#!forum/silverstripe-dev)
@@ -159,10 +163,9 @@ This ensures commits are easy to browse, and look nice on github.com
 As we automatically generate [changelogs](http://doc.silverstripe.org/sapphire/en/trunk/changelogs/) from them, we need a way to categorize and filter. 
 Please prefix **noteworthy** commit messages with one of the following tags: 
 
-* `NEW`: New feature or major enhancement (both for users and developers)
-* `API`: Addition of a new API, or modification/removal/deprecation of an existing API.
-  Includes any change developers should be aware of when upgrading.
-* `BUG`: Bugfix or minor enhancement on something developers or users are likely to encounter.
+* `NEW` New feature or major enhancement (both for users and developers)
+* `API` Addition of a new API, or modification/removal/deprecation of an existing API. Includes any change developers should be aware of when upgrading.
+* `BUG` Bugfix or minor enhancement on something developers or users are likely to encounter.
 
 All other commits should not be tagged if they are so trivial that most developers
 can ignore them during upgrades or when reviewing changes to the codebase.
@@ -195,16 +198,14 @@ Example: Good commit message
 
 ### Branch for new issue and develop on issue branch
 
-Before you start working on a new feature or bugfix, create a new branch dedicated to that one change named by issue number and description. If you're working on Issue #100, a retweet bugfix, create a new branch with the issue number and description, like this:
+Before you start working on a new feature or bugfix, create a new branch dedicated to that one change named by issue number and description. If you're working on Issue #100, a `DataObject::get_one()` bugfix, create a new branch with the issue number and description, like this:
 
-	$ git status
-	$ git branch 100-dataobject-get-one
-	$ git checkout 100-dataobject-get-one
+	$ git checkout -b 100-dataobject-get-one
 
 Edit and test the files on your development environment. When you've got something the way you want and established that it works, commit the changes to your branch on your local git repo. 
 
 	$ git add <filename>
-	$ git commit -m 'Issue #100: Some kind of descriptive message' 
+	$ git commit -m 'Some kind of descriptive message (fixes #100)' 
 
 You'll need to use git add for each file that you created or modified. There are ways to add multiple files, but I highly recommend a more deliberate approach unless you know what you're doing.
 
@@ -220,13 +221,13 @@ To keep your development branch up to date, rebase your changes on top of the cu
 
 If you've set up an upstream branch as detailed above, and a development branch called `100-dataobject-get-one`, you can update `upstream` and rebase your branch from it like so:
 
-    # [make sure all your changes are committed as necessary in branch]
+	# make sure all your changes are committed as necessary in branch
 	$ git fetch upstream
 	$ git rebase upstream/master
 
 Note that the example doesn't keep your own master branch up to date.  If you wanted to that, you might take the following approach instead:
 
-	# [make sure all your changes are committed as necessary in branch]
+	# make sure all your changes are committed as necessary in branch
 	$ git fetch upstream
 	$ git checkout master
 	$ git rebase upstream/master
@@ -248,11 +249,15 @@ To squash four commits into one, do the following:
 
 	$ git rebase -i upstream/master
 
-In the text editor that comes up, replace the words "pick" with "squash" next to the commits you want to squash into the commit before it. Save and close the editor, and git will combine the "squash"'ed commits with the one before it. Git will then give you the opportunity to change your commit message to something like, "BUGFIX Issue #100: Fixed DataObject::get_one() parameter order"
+In the text editor that comes up, replace the words "pick" with "squash" or just "s" next to the commits you want to squash into the commit before it.
+Save and close the editor, and git will combine the "squash"'ed commits with the one before it.
+Git will then give you the opportunity to change your commit message to something like, `BUG DataObject::get_one() parameter order (fixes #100)`.
+
+If you want to discard the commit messages from the commits you're squashing and just use the message from your "pick" commit(s) you can use "fixup" or "f" instead of "squash" to bypass the message editing and make the process a bit quicker.
 
 Important: If you've already pushed commits to GitHub, and then squash them locally, you will have to force-push to your GitHub again.  Add the `-f` argument to your git push command:
 
-    $ git push -f origin 100-dataobject-get-one
+	$ git push -f origin 100-dataobject-get-one
 
 Helpful hint: You can always edit your last commit message by using:
 
@@ -274,21 +279,23 @@ Sometimes, you might correct an issue which was reported in a different repo. In
 
 	$ git commit -m 'Issue silverstripe/silverstripe-cms#100: Some kind of descriptive message'
 
+Sometimes, you might correct an issue which was reported in a different repo. In these cases, don't simply refer to the issue number as GitHub will infer that as correcting an issue in the current repo. See [Commit Messages](code#commit-messages) above for the correct way to reference these issues.
+
 ## What is git rebase?
 
 Using `git rebase` helps create clean commit trees and makes keeping your code up-to-date with the current state of the upstream master easy. Here's how it works.
 
 Let's say you're working on Issue #212 a new plugin in your own branch and you start with something like this:
 
-          1---2---3 #212-my-new-plugin
-         /
-    A---B #master
+	      1---2---3 #212-my-new-plugin
+	     /
+	A---B #master
 
 You keep coding for a few days and then pull the latest upstream stuff and you end up like this:
 
-          1---2---3 #212-my-new-plugin
-         /
-    A---B--C--D--E--F #master
+	      1---2---3 #212-my-new-plugin
+	     /
+	A---B--C--D--E--F #master
 
 So all these new things (C,D,..F) have happened since you started. Normally you would just keep going (let's say you're not finished with the plugin yet) and then deal with a merge later on, which becomes a commit, which get moved upstream and ends up grafted on the tree forever.
 
@@ -298,9 +305,9 @@ git rebase master 212-my-new-plugin
 
 git will rewrite your commits like this:
 
-                      1---2---3 #212-my-new-plugin
-                     /
-    A---B--C--D--E--F #master
+	                  1---2---3 #212-my-new-plugin
+	                 /
+	A---B--C--D--E--F #master
 
 It's as if you had just started your branch. One immediate advantage you get is that you can test your branch now to see if C, D, E, or F had any impact on your code (you don't need to wait until you're finished with your plugin and merge to find this out). And, since you can keep doing this over and over again as you develop your plugin, at the end your merge will just be a fast-forward (in other words no merge at all).
 

forms/FormField.php

@@ -3,20 +3,22 @@
 /**
  * Represents a field in a form.
  *
- * A FieldList contains a number of FormField objects which make up the whole
- * of a form. In addition to single fields, FormField objects can be
- * "composite", for example, the {@link TabSet} field. Composite fields let us
- * define complex forms without having to resort to custom HTML.
+ * A FieldList contains a number of FormField objects which make up the whole of a form.
  *
- * <b>Subclassing</b>
+ * In addition to single fields, FormField objects can be "composite", for example, the
+ * {@link TabSet} field. Composite fields let us define complex forms without having to resort to
+ * custom HTML.
  *
- * Define a {@link dataValue()} method that returns a value suitable for
- * inserting into a single database field. For example, you might tidy up the
- * format of a date or currency field. Define {@link saveInto()} to totally
- * customise saving. For example, data might be saved to the filesystem instead
- * of the data record, or saved to a component of the data record instead of
- * the data record itself. Define {@link validate()} to validate the form field
- * and ensure that the content provided is valid.
+ * To subclass:
+ *
+ * Define a {@link dataValue()} method that returns a value suitable for inserting into a single
+ * database field.
+ *
+ * For example, you might tidy up the format of a date or currency field. Define {@link saveInto()}
+ * to totally customise saving.
+ *
+ * For example, data might be saved to the filesystem instead of the data record, or saved to a
+ * component of the data record instead of the data record itself.
  *
  * @package forms
  * @subpackage core
@@ -28,16 +30,49 @@ class FormField extends RequestHandler {
 	 */
 	protected $form;
 
-	protected $name, $title, $value ,$message, $messageType, $extraClass;
+	/**
+	 * @var string
+	 */
+	protected $name;
 
 	/**
-	 * @var $description string Adds a "title"-attribute to the markup.
+	 * @var null|string
+	 */
+	protected $title;
+
+	/**
+	 * @var mixed
+	 */
+	protected $value;
+
+	/**
+	 * @var string
+	 */
+	protected $message;
+
+	/**
+	 * @var string
+	 */
+	protected $messageType;
+
+	/**
+	 * @var string
+	 */
+	protected $extraClass;
+
+	/**
+	 * Adds a title attribute to the markup.
+	 *
+	 * @var string
+	 *
 	 * @todo Implement in all subclasses
 	 */
 	protected $description;
 
 	/**
-	 * @var $extraClasses array Extra CSS-classes for the formfield-container
+	 * Extra CSS classes for the FormField container.
+	 *
+	 * @var array
 	 */
 	protected $extraClasses;
 
@@ -47,85 +82,111 @@ class FormField extends RequestHandler {
 	 */
 	private static $default_classes = array();
 
+
+	/**
+	 * @var bool
+	 */
 	public $dontEscape;
 
 	/**
-	 * @var $rightTitle string Used in SmallFieldHolder to force a right-aligned label, or in FieldHolder
-	 * to create contextual label.
+	 * Right-aligned, contextual label for the field.
+	 *
+	 * @var string
 	 */
 	protected $rightTitle;
 
 	/**
-	 * @var $leftTitle string Used in SmallFieldHolder() to force a left-aligned label with correct spacing.
-	 * Please use $title for FormFields rendered with FieldHolder().
+	 * Left-aligned, contextual label for the field.
+	 *
+	 * @var string
 	 */
 	protected $leftTitle;
 
 	/**
 	 * Stores a reference to the FieldList that contains this object.
+	 *
 	 * @var FieldList
 	 */
 	protected $containerFieldList;
 
 	/**
-	 * @var boolean
+	 * @var bool
 	 */
 	protected $readonly = false;
 
 	/**
-	 * @var boolean
+	 * @var bool
 	 */
 	protected $disabled = false;
 
 	/**
-	 * @var string custom validation message for the Field
-	 */
-	protected $customValidationMessage = "";
-
-	/**
-	 * Name of the template used to render this form field. If not set, then
-	 * will look up the class ancestry for the first matching template where
-	 * the template name equals the class name.
-	 *
-	 * To explicitly use a custom template or one named other than the form
-	 * field see {@link setTemplate()}, {@link setFieldHolderTemplate()}
+	 * Custom validation message for the field.
 	 *
 	 * @var string
 	 */
-	protected
-		$template,
-		$fieldHolderTemplate,
-		$smallFieldHolderTemplate;
+	protected $customValidationMessage = '';
 
 	/**
-	 * @var array All attributes on the form field (not the field holder).
-	 * Partially determined based on other instance properties, please use {@link getAttributes()}.
+	 * Name of the template used to render this form field. If not set, then will look up the class
+	 * ancestry for the first matching template where the template name equals the class name.
+	 *
+	 * To explicitly use a custom template or one named other than the form field see
+	 * {@link setTemplate()}.
+	 *
+	 * @var string
+	 */
+	protected $template;
+
+	/**
+	 * Name of the template used to render this form field. If not set, then will look up the class
+	 * ancestry for the first matching template where the template name equals the class name.
+	 *
+	 * To explicitly use a custom template or one named other than the form field see
+	 * {@link setFieldHolderTemplate()}.
+	 *
+	 * @var string
+	 */
+	protected $fieldHolderTemplate;
+
+	/**
+	 * @var string
+	 */
+	protected $smallFieldHolderTemplate;
+
+	/**
+	 * All attributes on the form field (not the field holder).
+	 *
+	 * Partially determined based on other instance properties.
+	 *
+	 * @see getAttributes()
+	 *
+	 * @var array
 	 */
 	protected $attributes = array();
 
 	/**
-	 * Takes a fieldname and converts camelcase to spaced
-	 * words. Also resolves combined fieldnames with dot syntax
-	 * to spaced words.
+	 * Takes a field name and converts camelcase to spaced words. Also resolves combined field
+	 * names with dot syntax to spaced words.
 	 *
 	 * Examples:
+	 *
 	 * - 'TotalAmount' will return 'Total Amount'
 	 * - 'Organisation.ZipCode' will return 'Organisation Zip Code'
 	 *
 	 * @param string $fieldName
+	 *
 	 * @return string
 	 */
 	public static function name_to_label($fieldName) {
 		if(strpos($fieldName, '.') !== false) {
 			$parts = explode('.', $fieldName);
-			$label = $parts[count($parts)-2] . ' ' . $parts[count($parts)-1];
+
+			$label = $parts[count($parts) - 2] . ' ' . $parts[count($parts) - 1];
 		} else {
 			$label = $fieldName;
 		}
 
-		$label = preg_replace("/([a-z]+)([A-Z])/","$1 $2", $label);
-
-		return $label;
+		return preg_replace('/([a-z]+)([A-Z])/', '$1 $2', $label);
 	}
 
 	/**
@@ -133,41 +194,51 @@ class FormField extends RequestHandler {
 	 *
 	 * @param string $tag
 	 * @param array $attributes
-	 * @param mixed $content
+	 * @param null|string $content
 	 *
 	 * @return string
 	 */
 	public static function create_tag($tag, $attributes, $content = null) {
 		$preparedAttributes = '';
 
-		foreach($attributes as $k => $v) {
-			// Note: as indicated by the $k == value item here; the decisions over what to include in the attributes
-			// can sometimes get finicky
-			if(!empty($v) || $v === '0' || ($k == 'value' && $v !== null) ) {
-				$preparedAttributes .= " $k=\"" . Convert::raw2att($v) . "\"";
+		foreach($attributes as $attributeKey => $attributeValue) {
+			if(!empty($attributeValue) || $attributeValue === '0' || ($attributeKey == 'value' && $attributeValue !== null)) {
+				$preparedAttributes .= sprintf(
+					' %s="%s"', $attributeKey, Convert::raw2att($attributeValue)
+				);
 			}
 		}
 
 		if($content || $tag != 'input') {
-			return "<$tag$preparedAttributes>$content</$tag>";
-	}
-		else {
-			return "<$tag$preparedAttributes />";
+			return sprintf(
+				'<%s%s>%s</%s>', $tag, $preparedAttributes, $content, $tag
+			);
 		}
+
+		return sprintf(
+			'<%s%s />', $tag, $preparedAttributes
+		);
 	}
 
 	/**
 	 * Creates a new field.
 	 *
 	 * @param string $name The internal field name, passed to forms.
-	 * @param string $title The human-readable field label.
+	 * @param null|string $title The human-readable field label.
 	 * @param mixed $value The value of the field.
 	 */
 	public function __construct($name, $title = null, $value = null) {
 		$this->name = $name;
-		$this->title = ($title === null) ? self::name_to_label($name) : $title;
 
-		if($value !== NULL) $this->setValue($value);
+		if($title === null) {
+			$this->title = self::name_to_label($name);
+		} else {
+			$this->title = $title;
+		}
+
+		if($value !== null) {
+			$this->setValue($value);
+		}
 
 		parent::__construct();
 
@@ -175,7 +246,7 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * set up the default classes for the form. This is done on construct so that the default classes can be removed
+	 * Set up the default classes for the form. This is done on construct so that the default classes can be removed
 	 * after instantiation
 	 */
 	protected function setupDefaultClasses() {
@@ -199,10 +270,10 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Returns the HTML ID of the field - used in the template by label tags.
+	 * Returns the HTML ID of the field.
 	 *
-	 * The ID is generated as FormName_FieldName.  All Field functions should ensure
-	 * that this ID is included in the field.
+	 * The ID is generated as FormName_FieldName. All Field functions should ensure that this ID is
+	 * included in the field.
 	 *
 	 * @return string
 	 */
@@ -237,7 +308,7 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Returns the raw field name.
+	 * Returns the field name.
 	 *
 	 * @return string
 	 */
@@ -257,10 +328,11 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Returns the field message type, used by form validation.
+	 * Returns the field message type.
 	 *
-	 * Arbitrary value which is mostly used for CSS classes in the rendered HTML,
-	 * e.g. "required". Use {@link setError()} to set this property.
+	 * Arbitrary value which is mostly used for CSS classes in the rendered HTML, e.g "required".
+	 *
+	 * Use {@link setError()} to set this property.
 	 *
 	 * @return string
 	 */
@@ -269,7 +341,9 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Returns the field value - used by templates.
+	 * Returns the field value.
+	 *
+	 * @return mixed
 	 */
 	public function Value() {
 		return $this->value;
@@ -289,8 +363,7 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Returns the field value suitable for insertion into the
-	 * {@link DataObject}.
+	 * Returns the field value suitable for insertion into the data object.
 	 *
 	 * @return mixed
 	 */
@@ -308,12 +381,13 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * @param string $val
+	 * @param string $title
 	 *
-	 * @return FormField
+	 * @return $this
 	 */
-	public function setTitle($val) {
-		$this->title = $val;
+	public function setTitle($title) {
+		$this->title = $title;
+
 		return $this;
 	}
 
@@ -328,56 +402,78 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Sets the contextual label.
+	 * @param string $rightTitle
 	 *
-	 * @param $val string Text to set on the label.
+	 * @return $this
 	 */
-	public function setRightTitle($val) {
-		$this->rightTitle = $val;
-		return $this;
-	}
+	public function setRightTitle($rightTitle) {
+		$this->rightTitle = $rightTitle;
 
-	public function LeftTitle() {
-		return $this->leftTitle;
-	}
-
-	public function setLeftTitle($val) {
-		$this->leftTitle = $val;
 		return $this;
 	}
 
 	/**
-	 * Compiles all CSS-classes. Optionally includes a "nolabel"-class
-	 * if no title was set on the formfield.
-	 * Uses {@link Message()} and {@link MessageType()} to add validatoin
-	 * error classes which can be used to style the contained tags.
+	 * @return string
+	 */
+	public function LeftTitle() {
+		return $this->leftTitle;
+	}
+
+	/**
+	 * @param string $leftTitle
 	 *
-	 * @return string CSS-classnames
+	 * @return $this
+	 */
+	public function setLeftTitle($leftTitle) {
+		$this->leftTitle = $leftTitle;
+
+		return $this;
+	}
+
+	/**
+	 * Compiles all CSS-classes. Optionally includes a "nolabel" class if no title was set on the
+	 * FormField.
+	 *
+	 * Uses {@link Message()} and {@link MessageType()} to add validation error classes which can
+	 * be used to style the contained tags.
+	 *
+	 * @return string
 	 */
 	public function extraClass() {
 		$classes = array();
 
 		$classes[] = $this->Type();
 
-		if($this->extraClasses) $classes = array_merge($classes, array_values($this->extraClasses));
+		if($this->extraClasses) {
+			$classes = array_merge(
+				$classes,
+				array_values($this->extraClasses)
+			);
+		}
 
-		// Allow customization of label and field tag positioning
-		if(!$this->Title()) $classes[] = "nolabel";
+		if(!$this->Title()) {
+			$classes[] = 'nolabel';
+		}
 
-		// Allow custom styling of any element in the container based
-		// on validation errors, e.g. red borders on input tags.
-		// CSS-Class needs to be different from the one rendered
-		// through {@link FieldHolder()}
-		if($this->Message()) $classes[] .= "holder-" . $this->MessageType();
+		// Allow custom styling of any element in the container based on validation errors,
+		// e.g. red borders on input tags.
+		//
+		// CSS class needs to be different from the one rendered through {@link FieldHolder()}.
+		if($this->Message()) {
+			$classes[] .= 'holder-' . $this->MessageType();
+		}
 
 		return implode(' ', $classes);
 	}
 
 	/**
-	 * Add one or more CSS-classes to the formfield-container. Multiple class
-	 * names should be space delimited.
+	 * Add one or more CSS-classes to the FormField container.
+	 *
+	 * Multiple class names should be space delimited.
 	 *
 	 * @param string $class
+	 *
+	 * @return $this
 	 */
 	public function addExtraClass($class) {
 		$classes = preg_split('/\s+/', $class);
@@ -390,9 +486,11 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Remove one or more CSS-classes from the formfield-container.
+	 * Remove one or more CSS-classes from the FormField container.
 	 *
 	 * @param string $class
+	 *
+	 * @return $this
 	 */
 	public function removeExtraClass($class) {
 		$classes = preg_split('/\s+/', $class);
@@ -407,34 +505,43 @@ class FormField extends RequestHandler {
 	/**
 	 * Set an HTML attribute on the field element, mostly an <input> tag.
 	 *
-	 * Some attributes are best set through more specialized methods, to avoid interfering with built-in behaviour:
+	 * Some attributes are best set through more specialized methods, to avoid interfering with
+	 * built-in behaviour:
+	 *
 	 * - 'class': {@link addExtraClass()}
 	 * - 'title': {@link setDescription()}
 	 * - 'value': {@link setValue}
 	 * - 'name': {@link setName}
 	 *
-	 * CAUTION Doesn't work on most fields which are composed of more than one HTML form field:
-	 * AjaxUniqueTextField, CheckboxSetField, CompositeField, ConfirmedPasswordField,
-	 * CountryDropdownField, CreditCardField, CurrencyField, DateField, DatetimeField, FieldGroup, GridField,
-	 * HtmlEditorField, ImageField, ImageFormAction, InlineFormAction, ListBoxField, etc.
+	 * Caution: this doesn't work on most fields which are composed of more than one HTML form
+	 * field.
 	 *
-	 * @param string
-	 * @param string
+	 * @param string $name
+	 * @param string $value
+	 *
+	 * @return $this
 	 */
 	public function setAttribute($name, $value) {
 		$this->attributes[$name] = $value;
+
 		return $this;
 	}
 
 	/**
 	 * Get an HTML attribute defined by the field, or added through {@link setAttribute()}.
-	 * Caution: Doesn't work on all fields, see {@link setAttribute()}.
 	 *
-	 * @return string
+	 * Caution: this doesn't work on all fields, see {@link setAttribute()}.
+	 *
+	 * @return null|string
 	 */
 	public function getAttribute($name) {
-		$attrs = $this->getAttributes();
-		if(isset($attrs[$name])) return $attrs[$name];
+		$attributes = $this->getAttributes();
+
+		if(isset($attributes[$name])) {
+			return $attributes[$name];
+		}
+
+		return null;
 	}
 
 	/**
@@ -445,7 +552,7 @@ class FormField extends RequestHandler {
 	 * @return array
 	 */
 	public function getAttributes() {
-		$attrs = array(
+		$attributes = array(
 			'type' => 'text',
 			'name' => $this->getName(),
 			'value' => $this->Value(),
@@ -455,54 +562,67 @@ class FormField extends RequestHandler {
 			'readonly' => $this->isReadonly()
 		);
 
-		if ($this->Required()) {
-			$attrs['required'] = 'required';
-			$attrs['aria-required'] = 'true';
+		if($this->Required()) {
+			$attributes['required'] = 'required';
+			$attributes['aria-required'] = 'true';
 		}
 
-		$attrs = array_merge($attrs, $this->attributes);
+		$attributes = array_merge($attributes, $this->attributes);
 
-		$this->extend('updateAttributes', $attrs);
+		$this->extend('updateAttributes', $attributes);
 
-		return $attrs;
+		return $attributes;
 	}
 
 	/**
-	 * @param Array Custom attributes to process. Falls back to {@link getAttributes()}.
-	 * If at least one argument is passed as a string, all arguments act as excludes by name.
+	 * Custom attributes to process. Falls back to {@link getAttributes()}.
 	 *
-	 * @return string HTML attributes, ready for insertion into an HTML tag
+	 * If at least one argument is passed as a string, all arguments act as excludes, by name.
+	 *
+	 * @param array $attributes
+	 *
+	 * @return string
 	 */
-	public function getAttributesHTML($attrs = null) {
-		$exclude = (is_string($attrs)) ? func_get_args() : null;
+	public function getAttributesHTML($attributes = null) {
+		$exclude = null;
 
-		if(!$attrs || is_string($attrs)) {
-			$attrs = $this->getAttributes();
+		if(is_string($attributes)) {
+			$exclude = func_get_args();
 		}
 
-		// Remove empty
-		$attrs = array_filter((array)$attrs, function($v) {
+		if(!$attributes || is_string($attributes)) {
+			$attributes = $this->getAttributes();
+		}
+
+		$attributes = (array) $attributes;
+
+		$attributes = array_filter($attributes, function ($v) {
 			return ($v || $v === 0 || $v === '0');
 		});
 
-		// Remove excluded
 		if($exclude) {
-			$attrs = array_diff_key($attrs, array_flip($exclude));
+			$attributes = array_diff_key(
+				$attributes,
+				array_flip($exclude)
+			);
 		}
 
 		// Create markup
 		$parts = array();
 
-		foreach($attrs as $name => $value) {
-			$parts[] = ($value === true) ? "{$name}=\"{$name}\"" : "{$name}=\"" . Convert::raw2att($value) . "\"";
+		foreach($attributes as $name => $value) {
+			if($value === true) {
+				$parts[] = sprintf('%s="%s"', $name, $name);
+			} else {
+				$parts[] = sprintf('%s="%s"', $name, Convert::raw2att($value));
+			}
 		}
 
 		return implode(' ', $parts);
 	}
 
 	/**
-	 * Returns a version of a title suitable for insertion into an HTML
-	 * attribute.
+	 * Returns a version of a title suitable for insertion into an HTML attribute.
 	 *
 	 * @return string
 	 */
@@ -511,8 +631,7 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Returns a version of a title suitable for insertion into an HTML
-	 * attribute.
+	 * Returns a version of a title suitable for insertion into an HTML attribute.
 	 *
 	 * @return string
 	 */
@@ -524,8 +643,9 @@ class FormField extends RequestHandler {
 	 * Set the field value.
 	 *
 	 * @param mixed $value
-	 * @param mixed $data Optional data source passed in by {@see Form::loadDataFrom}
-	 * @return FormField Self reference
+	 * @param null|array|DataObject $data {@see Form::loadDataFrom}
+	 *
+	 * @return $this
 	 */
 	public function setValue($value) {
 		$this->value = $value;
@@ -534,11 +654,11 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Set the field name
+	 * Set the field name.
 	 *
 	 * @param string $name
 	 *
-	 * @return FormField
+	 * @return $this
 	 */
 	public function setName($name) {
 		$this->name = $name;
@@ -549,12 +669,11 @@ class FormField extends RequestHandler {
 	/**
 	 * Set the container form.
 	 *
-	 * This is called whenever you create a new form and put fields inside it,
-	 * so that you don't have to worry about linking the two.
+	 * This is called automatically when fields are added to forms.
 	 *
-	 * @param Form
+	 * @param Form $form
 	 *
-	 * @return FormField
+	 * @return $this
 	 */
 	public function setForm($form) {
 		$this->form = $form;
@@ -572,8 +691,7 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Return TRUE if security token protection is enabled on the parent
-	 * {@link Form}.
+	 * Return true if security token protection is enabled on the parent {@link Form}.
 	 *
 	 * @return bool
 	 */
@@ -588,10 +706,14 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * @param string $message Message to show to the user. Allows HTML content,
-	 *                        which means you need to use Convert::raw2xml() for any user supplied data.
+	 * Sets the error message to be displayed on the form field.
+	 *
+	 * Allows HTML content, so remember to use Convert::raw2xml().
+	 *
+	 * @param string $message
 	 * @param string $messageType
-	 * @return FormField
+	 *
+	 * @return $this
 	 */
 	public function setError($message, $messageType) {
 		$this->message = $message;
@@ -601,24 +723,23 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Set the custom error message to show instead of the default
-	 * format of Please Fill In XXX. Different from setError() as
-	 * that appends it to the standard error messaging.
+	 * Set the custom error message to show instead of the default format.
 	 *
-	 * @param string $msg Message for the error
+	 * Different from setError() as that appends it to the standard error messaging.
 	 *
-	 * @return FormField
+	 * @param string $customValidationMessage
+	 *
+	 * @return $this
 	 */
-	public function setCustomValidationMessage($msg) {
-		$this->customValidationMessage = $msg;
+	public function setCustomValidationMessage($customValidationMessage) {
+		$this->customValidationMessage = $customValidationMessage;
 
 		return $this;
 	}
 
 	/**
-	 * Get the custom error message for this form field. If a custom
-	 * message has not been defined then just return blank. The default
-	 * error is defined on {@link Validator}.
+	 * Get the custom error message for this form field. If a custom message has not been defined
+	 * then just return blank. The default error is defined on {@link Validator}.
 	 *
 	 * @return string
 	 */
@@ -629,12 +750,12 @@ class FormField extends RequestHandler {
 	/**
 	 * Set name of template (without path or extension).
 	 *
-	 * Caution: Not consistently implemented in all subclasses, please check
-	 * the {@link Field()} method on the subclass for support.
+	 * Caution: Not consistently implemented in all subclasses, please check the {@link Field()}
+	 * method on the subclass for support.
 	 *
 	 * @param string $template
 	 *
-	 * @return FormField
+	 * @return $this
 	 */
 	public function setTemplate($template) {
 		$this->template = $template;
@@ -657,18 +778,18 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Set name of template (without path or extension) for the holder,
-	 * which in turn is responsible for rendering {@link Field()}.
+	 * Set name of template (without path or extension) for the holder, which in turn is
+	 * responsible for rendering {@link Field()}.
 	 *
-	 * Caution: Not consistently implemented in all subclasses,
-	 * please check the {@link Field()} method on the subclass for support.
+	 * Caution: Not consistently implemented in all subclasses, please check the {@link Field()}
+	 * method on the subclass for support.
 	 *
-	 * @param string $template
+	 * @param string $fieldHolderTemplate
 	 *
-	 * @return FormField
+	 * @return $this
 	 */
-	public function setFieldHolderTemplate($template) {
-		$this->fieldHolderTemplate = $template;
+	public function setFieldHolderTemplate($fieldHolderTemplate) {
+		$this->fieldHolderTemplate = $fieldHolderTemplate;
 
 		return $this;
 	}
@@ -681,50 +802,67 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Set name of template (without path or extension) for the small holder,
-	 * which in turn is responsible for rendering {@link Field()}.
+	 * Set name of template (without path or extension) for the small holder, which in turn is
+	 * responsible for rendering {@link Field()}.
 	 *
-	 * Caution: Not consistently implemented in all subclasses,
-	 * please check the {@link Field()} method on the subclass for support.
+	 * Caution: Not consistently implemented in all subclasses, please check the {@link Field()}
+	 * method on the subclass for support.
 	 *
-	 * @param string
+	 * @param string $smallFieldHolderTemplate
+	 *
+	 * @return $this
 	 */
-	public function setSmallFieldHolderTemplate($template) {
-		$this->smallFieldHolderTemplate = $template;
+	public function setSmallFieldHolderTemplate($smallFieldHolderTemplate) {
+		$this->smallFieldHolderTemplate = $smallFieldHolderTemplate;
 
 		return $this;
 	}
 
 	/**
-	 * Returns the form field - used by templates.
+	 * Returns the form field.
+	 *
 	 * Although FieldHolder is generally what is inserted into templates, all of the field holder
-	 * templates make use of $Field.  It's expected that FieldHolder will give you the "complete"
+	 * templates make use of $Field. It's expected that FieldHolder will give you the "complete"
 	 * representation of the field on the form, whereas Field will give you the core editing widget,
 	 * such as an input tag.
 	 *
-	 * @param array $properties key value pairs of template variables
+	 * @param array $properties
+	 *
 	 * @return string
 	 */
 	public function Field($properties = array()) {
-		$obj = ($properties) ? $this->customise($properties) : $this;
+		$context = $this;
+
+		if(count($properties)) {
+			$context = $context->customise($properties);
+		}
+
 		$this->extend('onBeforeRender', $this);
-		return $obj->renderWith($this->getTemplates());
+
+		return $context->renderWith($this->getTemplates());
 	}
 
 	/**
-	 * Returns a "field holder" for this field - used by templates.
+	 * Returns a "field holder" for this field.
 	 *
 	 * Forms are constructed by concatenating a number of these field holders.
+	 *
 	 * The default field holder is a label and a form field inside a div.
+	 *
 	 * @see FieldHolder.ss
 	 *
-	 * @param array $properties key value pairs of template variables
+	 * @param array $properties
+	 *
 	 * @return string
 	 */
 	public function FieldHolder($properties = array()) {
-		$obj = ($properties) ? $this->customise($properties) : $this;
+		$context = $this;
 
-		return $obj->renderWith($this->getFieldHolderTemplates());
+		if(count($properties)) {
+			$context = $this->customise($properties);
+		}
+
+		return $context->renderWith($this->getFieldHolderTemplates());
 	}
 
 	/**
@@ -735,13 +873,17 @@ class FormField extends RequestHandler {
 	 * @return string
 	 */
 	public function SmallFieldHolder($properties = array()) {
-		$obj = ($properties) ? $this->customise($properties) : $this;
+		$context = $this;
 
-		return $obj->renderWith($this->getSmallFieldHolderTemplates());
+		if(count($properties)) {
+			$context = $this->customise($properties);
+		}
+
+		return $context->renderWith($this->getSmallFieldHolderTemplates());
 	}
 
 	/**
-	* Returns an array of templates to use for rendering {@link FieldH}
+	 * Returns an array of templates to use for rendering {@link FieldHolder}.
 	 *
 	 * @return array
 	 */
@@ -750,7 +892,7 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Returns an array of templates to use for rendering {@link FieldHolder}
+	 * Returns an array of templates to use for rendering {@link FieldHolder}.
 	 *
 	 * @return array
 	 */
@@ -762,7 +904,7 @@ class FormField extends RequestHandler {
 	}
 
 	/**
-	 * Returns an array of templates to use for rendering {@link SmallFieldHolder}
+	 * Returns an array of templates to use for rendering {@link SmallFieldHolder}.
 	 *
 	 * @return array
 	 */
@@ -775,31 +917,37 @@ class FormField extends RequestHandler {
 
 
 	/**
-	 * Generate an array of classname strings to use for rendering this form
-	 * field into HTML
+	 * Generate an array of class name strings to use for rendering this form field into HTML.
 	 *
-	 * @param string $custom custom template (if set)
-	 * @param string $suffix template suffix
+	 * @param string $customTemplate
+	 * @param string $customTemplateSuffix
 	 *
-	 * @return array $stack a stack of
+	 * @return array
 	 */
-	private function _templates($custom = null, $suffix = null) {
+	private function _templates($customTemplate = null, $customTemplateSuffix = null) {
 		$matches = array();
 
 		foreach(array_reverse(ClassInfo::ancestry($this)) as $className) {
-			$matches[] = $className . $suffix;
+			$matches[] = $className . $customTemplateSuffix;
 
-			if($className == "FormField") break;
+			if($className == "FormField") {
+				break;
+			}
 		}
 
-		if($custom) array_unshift($matches, $custom);
+		if($customTemplate) {
+			array_unshift($matches, $customTemplate);
+		}
 
 		return $matches;
 	}
 
 	/**
 	 * Returns true if this field is a composite field.
+	 *
 	 * To create composite field types, you should subclass {@link CompositeField}.
+	 *
+	 * @return bool
 	 */
 	public function isComposite() {
 		return false;
@@ -807,99 +955,138 @@ class FormField extends RequestHandler {
 
 	/**
 	 * Returns true if this field has its own data.
-	 * Some fields, such as titles and composite fields, don't actually have any data.  It doesn't
-	 * make sense for data-focused methods to look at them.  By overloading hasData() to return false,
-	 * you can prevent any data-focused methods from looking at it.
+	 *
+	 * Some fields, such as titles and composite fields, don't actually have any data. It doesn't
+	 * make sense for data-focused methods to look at them. By overloading hasData() to return
+	 * false, you can prevent any data-focused methods from looking at it.
 	 *
 	 * @see FieldList::collateDataFields()
+	 *
+	 * @return bool
 	 */
 	public function hasData() {
 		return true;
 	}
 
 	/**
-	 * @return boolean
+	 * @return bool
 	 */
 	public function isReadonly() {
 		return $this->readonly;
 	}
 
 	/**
-	 * Sets readonly-flag on form-field. Please use performReadonlyTransformation()
-	 * to actually transform this instance.
-	 * @param $bool boolean Setting "false" has no effect on the field-state.
+	 * Sets a read-only flag on this FormField.
+	 *
+	 * Use performReadonlyTransformation() to transform this instance.
+	 *
+	 * Setting this to false has no effect on the field.
+	 *
+	 * @param bool $readonly
+	 *
+	 * @return $this
 	 */
-	public function setReadonly($bool) {
-		$this->readonly = $bool;
+	public function setReadonly($readonly) {
+		$this->readonly = $readonly;
+
 		return $this;
 	}
 
 	/**
-	 * @return boolean
+	 * @return bool
 	 */
 	public function isDisabled() {
 		return $this->disabled;
 	}
 
 	/**
-	 * Sets disabed-flag on form-field. Please use performDisabledTransformation()
-	 * to actually transform this instance.
-	 * @param $bool boolean Setting "false" has no effect on the field-state.
+	 * Sets a disabled flag on this FormField.
+	 *
+	 * Use performDisabledTransformation() to transform this instance.
+	 *
+	 * Setting this to false has no effect on the field.
+	 *
+	 * @param bool $disabled
+	 *
+	 * @return $this
 	 */
-	public function setDisabled($bool) {
-		$this->disabled = $bool;
+	public function setDisabled($disabled) {
+		$this->disabled = $disabled;
+
 		return $this;
 	}
 
 	/**
-	 * Returns a readonly version of this field
+	 * Returns a read-only version of this field.
+	 *
+	 * @return FormField
 	 */
 	public function performReadonlyTransformation() {
-		$readonlyClassName = $this->class . '_Disabled';
+		$readonlyClassName = $this->class . '_Readonly';
+
 		if(ClassInfo::exists($readonlyClassName)) {
 			$clone = $this->castedCopy($readonlyClassName);
 		} else {
 			$clone = $this->castedCopy('ReadonlyField');
-			$clone->setReadonly(true);
-	}
+		}
+
+		$clone->setReadonly(true);
 
 		return $clone;
 	}
 
 	/**
 	 * Return a disabled version of this field.
-	 * Tries to find a class of the class name of this field suffixed with "_Disabled",
-	 * failing that, finds a method {@link setDisabled()}.
+	 *
+	 * Tries to find a class of the class name of this field suffixed with "_Disabled", failing
+	 * that, finds a method {@link setDisabled()}.
 	 *
 	 * @return FormField
 	 */
 	public function performDisabledTransformation() {
 		$disabledClassName = $this->class . '_Disabled';
+
 		if(ClassInfo::exists($disabledClassName)) {
 			$clone = $this->castedCopy($disabledClassName);
 		} else {
 			$clone = clone $this;
-			$clone->setDisabled(true);
 		}
 
-			return $clone;
-		}
+		$clone->setDisabled(true);
 
-	public function transform(FormTransformation $trans) {
-		return $trans->transform($this);
+		return $clone;
 	}
 
-	public function hasClass($class){
-		$patten = '/'.strtolower($class).'/i';
-		$subject = strtolower($this->class." ".$this->extraClass());
+	/**
+	 * @param FormTransformation $transformation
+	 *
+	 * @return mixed
+	 */
+	public function transform(FormTransformation $transformation) {
+		return $transformation->transform($this);
+	}
+
+	/**
+	 * @param string $class
+	 *
+	 * @return int
+	 */
+	public function hasClass($class) {
+		$patten = '/' . strtolower($class) . '/i';
+
+		$subject = strtolower($this->class . ' ' . $this->extraClass());
+
 		return preg_match($patten, $subject);
 	}
 
 	/**
-	 * Returns the field type - used by templates.
+	 * Returns the field type.
+	 *
 	 * The field type is the class name with the word Field dropped off the end, all lowercase.
-	 * It's handy for assigning HTML classes. Doesn't signify the <input type> attribute,
-	 * see {link getAttributes()}.
+	 *
+	 * It's handy for assigning HTML classes. Doesn't signify the <input type> attribute.
+	 *
+	 * @see {link getAttributes()}.
 	 *
 	 * @return string
 	 */
@@ -909,18 +1096,28 @@ class FormField extends RequestHandler {
 
 	/**
 	 * @deprecated 4.0 Use FormField::create_tag()
+	 *
+	 * @param string $tag
+	 * @param array $attributes
+	 * @param null|string $content
+	 *
+	 * @return string
 	 */
 	public function createTag($tag, $attributes, $content = null) {
 		Deprecation::notice('4.0', 'Use FormField::create_tag()');
+
 		return self::create_tag($tag, $attributes, $content);
 	}
 
 	/**
-	 * Validation method each {@link FormField} subclass should implement,
-	 * determining whether the field is valid or not based on the value.
+	 * Abstract method each {@link FormField} subclass must implement, determines whether the field
+	 * is valid or not based on the value.
+	 *
+	 * @todo Make this abstract.
 	 *
 	 * @param Validator $validator
-	 * @return boolean
+	 *
+	 * @return bool
 	 */
 	public function validate($validator) {
 		return true;
@@ -928,13 +1125,16 @@ class FormField extends RequestHandler {
 
 	/**
 	 * Describe this field, provide help text for it.
-	 * By default, renders as a <span class="description">
-	 * underneath the form field.
 	 *
-	 * @return string Description
+	 * By default, renders as a <span class="description"> underneath the form field.
+	 *
+	 * @param string $description
+	 *
+	 * @return $this
 	 */
 	public function setDescription($description) {
 		$this->description = $description;
+
 		return $this;
 	}
 
@@ -945,37 +1145,51 @@ class FormField extends RequestHandler {
 		return $this->description;
 	}
 
+	/**
+	 * @return string
+	 */
 	public function debug() {
-		return "$this->class ($this->name: $this->title : <font style='color:red;'>$this->message</font>)"
-			. " = $this->value";
+		return sprintf(
+			'%s (%s: %s : <span style="color:red;">%s</span>) = %s',
+			$this->class,
+			$this->name,
+			$this->title,
+			$this->message,
+			$this->value
+		);
 	}
 
 	/**
-	 * This function is used by the template processor.  If you refer to a field as a $ variable, it
+	 * This function is used by the template processor. If you refer to a field as a $ variable, it
 	 * will return the $Field value.
+	 *
+	 * @return string
 	 */
 	public function forTemplate() {
 		return $this->Field();
 	}
 
 	/**
-	 * @uses Validator->fieldIsRequired()
-	 * @return boolean
+	 * @return bool
 	 */
 	public function Required() {
 		if($this->form && ($validator = $this->form->Validator)) {
 			return $validator->fieldIsRequired($this->name);
 		}
+
+		return false;
 	}
 
 	/**
 	 * Set the FieldList that contains this field.
 	 *
-	 * @param FieldList $list
+	 * @param FieldList $containerFieldList
+	 *
 	 * @return FieldList
 	 */
-	public function setContainerFieldList($list) {
-		$this->containerFieldList = $list;
+	public function setContainerFieldList($containerFieldList) {
+		$this->containerFieldList = $containerFieldList;
+
 		return $this;
 	}
 
@@ -988,31 +1202,46 @@ class FormField extends RequestHandler {
 		return $this->containerFieldList;
 	}
 
+	/**
+	 * @return null|FieldList
+	 */
 	public function rootFieldList() {
-		if(is_object($this->containerFieldList)) return $this->containerFieldList->rootFieldList();
-		else user_error("rootFieldList() called on $this->class object without a containerFieldList", E_USER_ERROR);
+		if(is_object($this->containerFieldList)) {
+			return $this->containerFieldList->rootFieldList();
+		}
+
+		user_error(
+			"rootFieldList() called on $this->class object without a containerFieldList",
+			E_USER_ERROR
+		);
+
+		return null;
 	}
 
 	/**
-	 * Returns another instance of this field, but "cast" to a different class.
-	 * The logic tries to retain all of the instance properties,
-	 * and may be overloaded by subclasses to set additional ones.
+	 * Returns another instance of this field, but "cast" to a different class. The logic tries to
+	 * retain all of the instance properties, and may be overloaded by subclasses to set additional
+	 * ones.
 	 *
-	 * Assumes the standard FormField parameter signature with
-	 * its name as the only mandatory argument. Mainly geared towards
-	 * creating *_Readonly or *_Disabled subclasses of the same type,
-	 * or casting to a {@link ReadonlyField}.
+	 * Assumes the standard FormField parameter signature with its name as the only mandatory
+	 * argument. Mainly geared towards creating *_Readonly or *_Disabled subclasses of the same
+	 * type, or casting to a {@link ReadonlyField}.
 	 *
-	 * Does not copy custom field templates, since they probably won't apply to
-	 * the new instance.
+	 * Does not copy custom field templates, since they probably won't apply to the new instance.
+	 *
+	 * @param mixed $classOrCopy Class name for copy, or existing copy instance to update
 	 *
-	 * @param  String $classOrCopy Class name for copy, or existing copy instance to update
 	 * @return FormField
 	 */
 	public function castedCopy($classOrCopy) {
-		$field = (is_object($classOrCopy)) ? $classOrCopy : new $classOrCopy($this->name);
+		$field = $classOrCopy;
+
+		if(!is_object($field)) {
+			$field = new $classOrCopy($this->name);
+		}
+
 		$field
-			->setValue($this->value) // get value directly from property, avoid any conversions
+			->setValue($this->value)
 			->setForm($this->form)
 			->setTitle($this->Title())
 			->setLeftTitle($this->LeftTitle())
@@ -1020,12 +1249,12 @@ class FormField extends RequestHandler {
 			->addExtraClass($this->extraClass())
 			->setDescription($this->getDescription());
 
-		// Only include built-in attributes, ignore anything
-		// set through getAttributes(), since those might change important characteristics
-		// of the field, e.g. its "type" attribute.
-		foreach($this->attributes as $k => $v) {
-			$field->setAttribute($k, $v);
-}
+		// Only include built-in attributes, ignore anything set through getAttributes().
+		// Those might change important characteristics of the field, e.g. its "type" attribute.
+		foreach($this->attributes as $attributeKey => $attributeValue) {
+			$field->setAttribute($attributeKey, $attributeValue);
+		}
+
 		$field->dontEscape = $this->dontEscape;
 
 		return $field;

i18n/i18nEntityProvider.php

@@ -9,13 +9,16 @@
  * of your values.
  * For any statics containing natural language, never use the static directly -
  * always wrap it in a getter.
- *
+ * 
+ * Classes must be able to be constructed without mandatory arguments, otherwise
+ * this interface will have no effect.
+ * 
  * @package framework
  * @subpackage i18n
  * @uses i18nTextCollector->collectFromEntityProviders()
  */
 interface i18nEntityProvider {
-
+	
 	/**
 	 * Example usage:
 	 * <code>
@@ -25,13 +28,13 @@ interface i18nEntityProvider {
 	 * 	foreach($this->stat('my_static_array) as $key => $value) {
 	 * 		$entities["MyTestClass.my_static_array_{$key}"] = array(
 	 * 			$value,
-	 *
+	 * 			
 	 * 			'My context description'
 	 * 		);
 	 * 	}
 	 * 	return $entities;
 	 * }
-	 *
+	 * 
 	 * public static function my_static_array() {
 	 * 	$t_my_static_array = array();
 	 * 	foreach(self::$my_static_array as $k => $v) {
@@ -41,9 +44,9 @@ interface i18nEntityProvider {
 	 * }
 	 * }
 	 * </code>
-	 *
+	 * 
 	 * Example usage in {@link DataObject->provideI18nEntities()}.
-	 *
+	 * 
 	 * You can ask textcollector to add the provided entity to a different module
 	 * than the class is contained in by adding a 4th argument to the array:
 	 * <code>
@@ -52,7 +55,7 @@ interface i18nEntityProvider {
 	 * 	$entities = array();
 	 * 		$entities["MyOtherModuleClass.MYENTITY"] = array(
 	 * 			$value,
-	 *
+	 * 			
 	 * 			'My context description',
 	 * 			'myothermodule'
 	 * 		);
@@ -60,7 +63,7 @@ interface i18nEntityProvider {
 	 * 	return $entities;
 	 * }
 	 * </code>
-	 *
+	 * 
 	 * @return array All entites in an associative array, with
 	 * entity name as the key, and a numerical array of pseudo-arguments
 	 * for _t() as a value.

i18n/i18nTextCollector.php

@@ -27,15 +27,28 @@
  */
 class i18nTextCollector extends Object {
 
+	/**
+	 * Default (master) locale
+	 *
+	 * @var string
+	 */
 	protected $defaultLocale;
 
 	/**
-	 * @var string $basePath The directory base on which the collector should act.
+	 * The directory base on which the collector should act.
 	 * Usually the webroot set through {@link Director::baseFolder()}.
+	 *
 	 * @todo Fully support changing of basePath through {@link SSViewer} and {@link ManifestBuilder}
+	 *
+	 * @var string
 	 */
 	public $basePath;
 
+	/**
+	 * Save path
+	 *
+	 * @var string
+	 */
 	public $baseSavePath;
 
 	/**
@@ -43,99 +56,267 @@ class i18nTextCollector extends Object {
 	 */
 	protected $writer;
 
+	/**
+	 * List of file extensions to parse
+	 *
+	 * @var array
+	 */
+	protected $fileExtensions = array('php', 'ss');
+
 	/**
 	 * @param $locale
 	 */
 	public function __construct($locale = null) {
-		$this->defaultLocale = ($locale) ? $locale : i18n::get_lang_from_locale(i18n::default_locale());
+		$this->defaultLocale = $locale
+			? $locale
+			: i18n::get_lang_from_locale(i18n::default_locale());
 		$this->basePath = Director::baseFolder();
 		$this->baseSavePath = Director::baseFolder();
 
 		parent::__construct();
 	}
 
+	/**
+	 * Assign a writer
+	 *
+	 * @param i18nTextCollector_Writer $writer
+	 */
 	public function setWriter($writer) {
 		$this->writer = $writer;
 	}
 
+	/**
+	 * Gets the currently assigned writer, or the default if none is specified.
+	 *
+	 * @return i18nTextCollector_Writer
+	 */
 	public function getWriter() {
-		if(!$this->writer) $this->writer = new i18nTextCollector_Writer_RailsYaml();
+		if(!$this->writer) {
+			$this->setWriter(Injector::inst()->get('i18nTextCollector_Writer'));
+		}
 		return $this->writer;
 	}
 
 	/**
-	 * This is the main method to build the master string tables with the original strings.
-	 * It will search for existent modules that use the i18n feature, parse the _t() calls
-	 * and write the resultant files in the lang folder of each module.
+	 * This is the main method to build the master string tables with the
+	 * original strings. It will search for existent modules that use the
+	 * i18n feature, parse the _t() calls and write the resultant files
+	 * in the lang folder of each module.
 	 *
 	 * @uses DataObject->collectI18nStatics()
 	 *
 	 * @param array $restrictToModules
-	 * @param array $mergeWithExisting Merge new master strings with existing ones
-	 * already defined in language files, rather than replacing them. This can be useful
-	 * for long-term maintenance of translations across releases, because it allows
-	 * "translation backports" to older releases without removing strings these older releases
-	 * still rely on.
+	 * @param bool $mergeWithExisting Merge new master strings with existing
+	 * ones already defined in language files, rather than replacing them.
+	 * This can be useful for long-term maintenance of translations across
+	 * releases, because it allows "translation backports" to older releases
+	 * without removing strings these older releases still rely on.
 	 */
 	public function run($restrictToModules = null, $mergeWithExisting = false) {
 		$entitiesByModule = $this->collect($restrictToModules, $mergeWithExisting);
+		if(empty($entitiesByModule)) {
+			return;
+		}
+
 		// Write each module language file
-		if($entitiesByModule) foreach($entitiesByModule as $module => $entities) {
-			$this->getWriter()->write($entities, $this->defaultLocale, $this->baseSavePath . '/' . $module);
+		foreach($entitiesByModule as $module => $entities) {
+			// Skip empty translations
+			if(empty($entities)) {
+				continue;
+			}
+
+			// Clean sorting prior to writing
+			ksort($entities);
+			$path = $this->baseSavePath . '/' . $module;
+			$this->getWriter()->write($entities, $this->defaultLocale, $path);
 		}
 	}
 
-	public function collect($restrictToModules = null, $mergeWithExisting = false) {
-		$modules = scandir($this->basePath);
-		$themeFolders = array();
+	/**
+	 * Gets the list of modules in this installer
+	 *
+	 * @param string $directory Path to look in
+	 * @return array List of modules as paths relative to base
+	 */
+	protected function getModules($directory) {
+		// Include self as head module
+		$modules = array();
 
-		// A master string tables array (one mst per module)
-		$entitiesByModule = array();
+		// Get all standard modules
+		foreach(glob($directory."/*", GLOB_ONLYDIR) as $path) {
+			// Check for _config
+			if(!is_file("$path/_config.php") && !is_dir("$path/_config")) {
+				continue;
+			}
+			$modules[] = basename($path);
+		}
 
-		foreach($modules as $index => $module){
-			if($module != 'themes') continue;
-			else {
-				$themes = scandir($this->basePath."/themes");
-				if(count($themes)){
-					foreach($themes as $theme) {
-						if(is_dir($this->basePath."/themes/".$theme)
-								&& substr($theme,0,1) != '.'
-								&& is_dir($this->basePath."/themes/".$theme."/templates")){
-
-							$themeFolders[] = 'themes/'.$theme;
-						}
-					}
-				}
-				$themesInd = $index;
+		// Get all themes
+		foreach(glob($directory."/themes/*", GLOB_ONLYDIR) as $path) {
+			// Check for templates
+			if(is_dir("$path/templates")) {
+				$modules[] = 'themes/'.basename($path);
 			}
 		}
 
-		if(isset($themesInd)) {
-			unset($modules[$themesInd]);
+		return $modules;
+	}
+
+	/**
+	 * Extract all strings from modules and return these grouped by module name
+	 *
+	 * @param array $restrictToModules
+	 * @param bool $mergeWithExisting
+	 * @return array
+	 */
+	public function collect($restrictToModules = array(), $mergeWithExisting = false) {
+		$entitiesByModule = $this->getEntitiesByModule();
+
+		// Resolve conflicts between duplicate keys across modules
+		$entitiesByModule = $this->resolveDuplicateConflicts($entitiesByModule);
+
+		// Optionally merge with existing master strings
+		if($mergeWithExisting) {
+			$entitiesByModule = $this->mergeWithExisting($entitiesByModule);
 		}
 
-		$modules = array_merge($modules, $themeFolders);
+		// Restrict modules we update to just the specified ones (if any passed)
+		if(!empty($restrictToModules)) {
+			foreach (array_diff(array_keys($entitiesByModule), $restrictToModules) as $module) {
+				unset($entitiesByModule[$module]);
+			}
+		}
+		return $entitiesByModule;
+	}
 
-		foreach($modules as $module) {
-			// Only search for calls in folder with a _config.php file or _config folder
-			// (which means they are modules, including themes folder)
-			$isValidModuleFolder = (
-				is_dir("$this->basePath/$module")
-				&& substr($module,0,1) != '.'
-				&& (
-					is_file("$this->basePath/$module/_config.php")
-					|| is_dir("$this->basePath/$module/_config")
-				)
-			) || (
-				substr($module,0,7) == 'themes/'
-				&& is_dir("$this->basePath/$module")
+	/**
+	 * Resolve conflicts between duplicate keys across modules
+	 *
+	 * @param array $entitiesByModule List of all modules with keys
+	 * @return array Filtered listo of modules with duplicate keys unassigned
+	 */
+	protected function resolveDuplicateConflicts($entitiesByModule) {
+		// Find all keys that exist across multiple modules
+		$conflicts = $this->getConflicts($entitiesByModule);
+		foreach($conflicts as $conflict) {
+			// Determine if we can narrow down the ownership
+			$bestModule = $this->getBestModuleForKey($entitiesByModule, $conflict);
+			if(!$bestModule) {
+				continue;
+			}
+
+			// Remove foreign duplicates
+			foreach($entitiesByModule as $module => $entities) {
+				if($module !== $bestModule) {
+					unset($entitiesByModule[$module][$conflict]);
+				}
+			}
+		}
+		return $entitiesByModule;
+	}
+
+	/**
+	 * Find all keys in the entity list that are duplicated across modules
+	 *
+	 * @param array $entitiesByModule
+	 * @return array List of keys
+	 */
+	protected function getConflicts($entitiesByModule) {
+		$modules = array_keys($entitiesByModule);
+		$allConflicts = array();
+		// bubble-compare each group of modules
+		for($i = 0; $i < count($modules) - 1; $i++) {
+			$left = array_keys($entitiesByModule[$modules[$i]]);
+			for($j = $i+1; $j < count($modules); $j++) {
+				$right = array_keys($entitiesByModule[$modules[$j]]);
+				$conflicts = array_intersect($left, $right);
+				$allConflicts = array_merge($allConflicts, $conflicts);
+			}
+		}
+		return array_unique($allConflicts);
+	}
+
+	/**
+	 * Determine the best module to be given ownership over this key
+	 *
+	 * @param array $entitiesByModule
+	 * @param string $key
+	 * @return string Best module, if found
+	 */
+	protected function getBestModuleForKey($entitiesByModule, $key) {
+		// Check classes
+		$class = current(explode('.', $key));
+		$owner = i18n::get_owner_module($class);
+		if($owner) {
+			return $owner;
+		}
+
+		// @todo - How to determine ownership of templates? Templates can
+		// exist in multiple locations with the same name.
+
+		// Display notice if not found
+		Debug::message(
+			"Duplicate key {$key} detected in multiple modules with no obvious owner",
+			false
+		);
+
+		// Fall back to framework then cms modules
+		foreach(array('framework', 'cms') as $module) {
+			if(isset($entitiesByModule[$module][$key])) {
+				return $module;
+			}
+		}
+
+		// Do nothing
+		return null;
+	}
+
+	/**
+	 * Merge all entities with existing strings
+	 *
+	 * @param array $entitiesByModule
+	 * @return array
+	 */
+	protected function mergeWithExisting($entitiesByModule) {
+		// TODO Support all defined source formats through i18n::get_translators().
+		//      Currently not possible because adapter instances can't be fully reset through the Zend API,
+		//      meaning master strings accumulate across modules
+		foreach($entitiesByModule as $module => $entities) {
+			$adapter = Injector::inst()->create('i18nRailsYamlAdapter');
+			$fileName = $adapter->getFilenameForLocale($this->defaultLocale);
+			$masterFile = "{$this->basePath}/{$module}/lang/{$fileName}";
+			if(!file_exists($masterFile)) {
+				continue;
+			}
+
+			$adapter->addTranslation(array(
+				'content' => $masterFile,
+				'locale' => $this->defaultLocale
+			));
+			$entitiesByModule[$module] = array_merge(
+				array_map(
+					// Transform each master string from scalar value to array of strings
+					function($v) {return array($v);},
+					$adapter->getMessages($this->defaultLocale)
+				),
+				$entities
 			);
+		}
+		return $entitiesByModule;
+	}
 
-			if(!$isValidModuleFolder) continue;
-
+	/**
+	 * Collect all entities grouped by module
+	 *
+	 * @return array
+	 */
+	protected function getEntitiesByModule() {
+		// A master string tables array (one mst per module)
+		$entitiesByModule = array();
+		$modules = $this->getModules($this->basePath);
+		foreach($modules as $module) {
 			// we store the master string tables
 			$processedEntities = $this->processModule($module);
-
 			if(isset($entitiesByModule[$module])) {
 				$entitiesByModule[$module] = array_merge_recursive($entitiesByModule[$module], $processedEntities);
 			} else {
@@ -143,54 +324,23 @@ class i18nTextCollector extends Object {
 			}
 
 			// extract all entities for "foreign" modules (fourth argument)
+			// @see CMSMenu::provideI18nEntities for an example usage
 			foreach($entitiesByModule[$module] as $fullName => $spec) {
-				if(isset($spec[2]) && $spec[2] && $spec[2] != $module) {
+				if(!empty($spec[2]) && $spec[2] !== $module) {
 					$othermodule = $spec[2];
-					if(!isset($entitiesByModule[$othermodule])) $entitiesByModule[$othermodule] = array();
+					if(!isset($entitiesByModule[$othermodule])) {
+						$entitiesByModule[$othermodule] = array();
+					}
 					unset($spec[2]);
 					$entitiesByModule[$othermodule][$fullName] = $spec;
 					unset($entitiesByModule[$module][$fullName]);
 				}
 			}
-
-			// Optionally merge with existing master strings
-			// TODO Support all defined source formats through i18n::get_translators().
-			//      Currently not possible because adapter instances can't be fully reset through the Zend API,
-			//      meaning master strings accumulate across modules
-			if($mergeWithExisting) {
-				$adapter = Injector::inst()->create(
-					'i18nRailsYamlAdapter',
-					array('locale' => 'auto')
-				);
-				$masterFile = "{$this->basePath}/{$module}/lang/"
-					. $adapter->getFilenameForLocale($this->defaultLocale);
-				if(!file_exists($masterFile)) continue;
-
-				$adapter->addTranslation(array(
-					'content' => $masterFile,
-					'locale' => $this->defaultLocale
-				));
-				$entitiesByModule[$module] = array_merge(
-					array_map(
-						// Transform each master string from scalar value to array of strings
-						function($v) {return array($v);},
-						$adapter->getMessages($this->defaultLocale)
-					),
-					$entitiesByModule[$module]
-				);
-			}
 		}
-
-		// Restrict modules we update to just the specified ones (if any passed)
-		if($restrictToModules && count($restrictToModules)) {
-			foreach (array_diff(array_keys($entitiesByModule), $restrictToModules) as $module) {
-				unset($entitiesByModule[$module]);
-			}
-		}
-
 		return $entitiesByModule;
 	}
 
+
 	public function write($module, $entities) {
 		$this->getWriter()->write($entities, $this->defaultLocale, $this->baseSavePath . '/' . $module);
 		return $this;
@@ -200,38 +350,31 @@ class i18nTextCollector extends Object {
 	 * Builds a master string table from php and .ss template files for the module passed as the $module param
 	 * @see collectFromCode() and collectFromTemplate()
 	 *
-	 * @param string $module A module's name or just 'themes'
-	 * @return array $entities An array of entities found in the files that comprise the module
-	 * @todo Why the type juggling for $this->collectFromBlah()? They always return arrays.
+	 * @param string $module A module's name or just 'themes/<themename>'
+	 * @return array An array of entities found in the files that comprise the module
 	 */
 	protected function processModule($module) {
 		$entities = array();
 
 		// Search for calls in code files if these exists
-		$fileList = array();
-		if(is_dir("$this->basePath/$module/code")) {
-			$fileList = $this->getFilesRecursive("$this->basePath/$module/code");
-		} else if($module == FRAMEWORK_DIR || substr($module, 0, 7) == 'themes/') {
-			// framework doesn't have the usual module structure, so we'll scan all subfolders
-			$fileList = $this->getFilesRecursive("$this->basePath/$module", null, null, '/\/(tests|dev)$/');
-		}
+		$fileList = $this->getFileListForModule($module);
 		foreach($fileList as $filePath) {
-			// exclude ss-templates, they're scanned separately
-			if(substr($filePath,-3) == 'php') {
-				$content = file_get_contents($filePath);
-				$entities = array_merge($entities,(array)$this->collectFromCode($content, $module));
-				$entities = array_merge($entities, (array)$this->collectFromEntityProviders($filePath, $module));
-			}
-		}
-
-		// Search for calls in template files if these exists
-		if(is_dir("$this->basePath/$module/")) {
-			$fileList = $this->getFilesRecursive("$this->basePath/$module/", null, 'ss');
-			foreach($fileList as $index => $filePath) {
-				$content = file_get_contents($filePath);
+			$extension = pathinfo($filePath, PATHINFO_EXTENSION);
+			$content = file_get_contents($filePath);
+			// Filter based on extension
+			if($extension === 'php') {
+				$entities = array_merge(
+					$entities,
+					$this->collectFromCode($content, $module),
+					$this->collectFromEntityProviders($filePath, $module)
+				);
+			} elseif($extension === 'ss') {
 				// templates use their filename as a namespace
 				$namespace = basename($filePath);
-				$entities = array_merge($entities, (array)$this->collectFromTemplate($content, $module, $namespace));
+				$entities = array_merge(
+					$entities,
+					$this->collectFromTemplate($content, $module, $namespace)
+				);
 			}
 		}
 
@@ -241,11 +384,44 @@ class i18nTextCollector extends Object {
 		return $entities;
 	}
 
+	/**
+	 * Retrieves the list of files for this module
+	 *
+	 * @param type $module
+	 * @return array List of files to parse
+	 */
+	protected function getFileListForModule($module) {
+		$modulePath = "{$this->basePath}/{$module}";
+
+		// Search all .ss files in themes
+		if(stripos($module, 'themes/') === 0) {
+			return $this->getFilesRecursive($modulePath, null, 'ss');
+		}
+
+		// If Framework or non-standard module structure, so we'll scan all subfolders
+		if($module === FRAMEWORK_DIR || !is_dir("{$modulePath}/code")) {
+			return $this->getFilesRecursive($modulePath);
+		}
+
+		// Get code files
+		$files = $this->getFilesRecursive("{$modulePath}/code", null, 'php');
+
+		// Search for templates in this module
+		if(is_dir("{$modulePath}/templates")) {
+			$templateFiles = $this->getFilesRecursive("{$modulePath}/templates", null, 'ss');
+		} else {
+			$templateFiles = $this->getFilesRecursive($modulePath, null, 'ss');
+		}
+
+		return array_merge($files, $templateFiles);
+	}
+
 	/**
 	 * Extracts translatables from .php files.
 	 *
 	 * @param string $content The text content of a parsed template-file
-	 * @param string $module Module's name or 'themes'
+	 * @param string $module Module's name or 'themes'. Could also be a namespace
+	 * Generated by templates includes. E.g. 'UploadField.ss'
 	 * @return array $entities An array of entities representing the extracted translation function calls in code
 	 */
 	public function collectFromCode($content, $module) {
@@ -328,33 +504,10 @@ class i18nTextCollector extends Object {
 	 * @param string $module Module's name or 'themes'
 	 * @param string $fileName The name of a template file when method is used in self-referencing mode
 	 * @return array $entities An array of entities representing the extracted template function calls
-	 *
-	 * @todo Why the type juggling for $this->collectFromTemplate()? It always returns an array.
 	 */
 	public function collectFromTemplate($content, $fileName, $module, &$parsedFiles = array()) {
-		$entities = array();
-
-		// Search for included templates
-		preg_match_all('/<' . '% include +([A-Za-z0-9_]+) +%' . '>/', $content, $regs, PREG_SET_ORDER);
-		foreach($regs as $reg) {
-			$includeName = $reg[1];
-			$includeFileName = "{$includeName}.ss";
-			$filePath = SSViewer::getTemplateFileByType($includeName, 'Includes');
-			if(!$filePath) $filePath = SSViewer::getTemplateFileByType($includeName, 'main');
-			if($filePath && !in_array($filePath, $parsedFiles)) {
-				$parsedFiles[] = $filePath;
-				$includeContent = file_get_contents($filePath);
-				$entities = array_merge(
-					$entities,
-					(array)$this->collectFromTemplate($includeContent, $module, $includeFileName, $parsedFiles)
-				);
-			}
-			// @todo Will get massively confused if you include the includer -> infinite loop
-		}
-
 		// use parser to extract <%t style translatable entities
-		$translatables = i18nTextCollector_Parser::GetTranslatables($content);
-		$entities = array_merge($entities,(array)$translatables);
+		$entities = i18nTextCollector_Parser::GetTranslatables($content);
 
 		// use the old method of getting _t() style translatable entities
 		// Collect in actual template
@@ -374,31 +527,34 @@ class i18nTextCollector extends Object {
 	}
 
 	/**
+	 * Allows classes which implement i18nEntityProvider to provide
+	 * additional translation strings.
+	 *
+	 * Not all classes can be instanciated without mandatory arguments,
+	 * so entity collection doesn't work for all SilverStripe classes currently
+	 *
 	 * @uses i18nEntityProvider
+	 * @param string $filePath
+	 * @param string $module
+	 * @return array
 	 */
 	public function collectFromEntityProviders($filePath, $module = null) {
 		$entities = array();
-
-		// HACK Ugly workaround to avoid "Cannot redeclare class PHPUnit_Framework_TestResult" error
-		// when running text collector with PHPUnit 3.4. There really shouldn't be any dependencies
-		// here, but the class reflection enforces autloading of seemingly unrelated classes.
-		// The main problem here is the CMSMenu class, which iterates through test classes,
-		// which in turn trigger autoloading of PHPUnit.
-		$phpunitwrapper = PhpUnitWrapper::inst();
-		$phpunitwrapper->init();
-
 		$classes = ClassInfo::classes_for_file($filePath);
-		if($classes) foreach($classes as $class) {
-			// Not all classes can be instanciated without mandatory arguments,
-			// so entity collection doesn't work for all SilverStripe classes currently
-			// Requires PHP 5.1+
-			if(class_exists($class) && in_array('i18nEntityProvider', class_implements($class))) {
-				$reflectionClass = new ReflectionClass($class);
-				if($reflectionClass->isAbstract()) continue;
-
-				$obj = singleton($class);
-				$entities = array_merge($entities,(array)$obj->provideI18nEntities());
+		foreach($classes as $class) {
+			// Skip non-implementing classes
+			if(!class_exists($class) || !in_array('i18nEntityProvider', class_implements($class))) {
+				continue;
 			}
+
+			// Skip abstract classes
+			$reflectionClass = new ReflectionClass($class);
+			if($reflectionClass->isAbstract()) {
+				continue;
+			}
+
+			$obj = singleton($class);
+			$entities = array_merge($entities, (array)$obj->provideI18nEntities());
 		}
 
 		ksort($entities);
@@ -442,30 +598,35 @@ class i18nTextCollector extends Object {
 	 *
 	 * @param string $folder base directory to scan (will scan recursively)
 	 * @param array $fileList Array to which potential files will be appended
-	 * @param string $type Optional, "php" or "ss"
-	 * @param  string $folderExclude Regular expression matching folder names to exclude
+	 * @param string $type Optional, "php" or "ss" only
+	 * @param string $folderExclude Regular expression matching folder names to exclude
 	 * @return array $fileList An array of files
 	 */
-	protected function getFilesRecursive($folder, $fileList = null, $type = null, $folderExclude = null) {
-		if(!$folderExclude) $folderExclude = '/\/(tests)$/';
-		if(!$fileList) $fileList = array();
-		$items = scandir($folder);
-		$isValidFolder = (
-			!in_array('_manifest_exclude', $items)
-			&& !preg_match($folderExclude, $folder)
-		);
+	protected function getFilesRecursive($folder, $fileList = array(), $type = null, $folderExclude = '/\/(tests)$/') {
+		if(!$fileList) {
+			$fileList = array();
+		}
+		// Skip ignored folders
+		if(is_file("{$folder}/_manifest_exclude") || preg_match($folderExclude, $folder)) {
+			return $fileList;
+		}
 
-		if($items && $isValidFolder) foreach($items as $item) {
-			if(substr($item,0,1) == '.') continue;
-			if(substr($item,-4) == '.php' && (!$type || $type == 'php')) {
-				$fileList[substr($item,0,-4)] = "$folder/$item";
-			} else if(substr($item,-3) == '.ss' && (!$type || $type == 'ss')) {
-				$fileList[$item] = "$folder/$item";
-			} else if(is_dir("$folder/$item")) {
+		foreach(glob($folder.'/*') as $path) {
+			// Recurse if directory
+			if(is_dir($path)) {
 				$fileList = array_merge(
 					$fileList,
-					$this->getFilesRecursive("$folder/$item", $fileList, $type, $folderExclude)
+					$this->getFilesRecursive($path, $fileList, $type, $folderExclude)
 				);
+				continue;
+			}
+
+			// Check if this extension is included
+			$extension = pathinfo($path, PATHINFO_EXTENSION);
+			if(in_array($extension, $this->fileExtensions)
+				&& (!$type || $type === $extension)
+			) {
+				$fileList[$path] = $path;
 			}
 		}
 		return $fileList;
@@ -691,7 +852,9 @@ class i18nTextCollector_Parser extends SSTemplateParser {
 
 		// Run the parser and throw away the result
 		$parser = new i18nTextCollector_Parser($template);
-		if(substr($template, 0,3) == pack("CCC", 0xef, 0xbb, 0xbf)) $parser->pos = 3;
+		if(substr($template, 0,3) == pack("CCC", 0xef, 0xbb, 0xbf)) {
+			$parser->pos = 3;
+		}
 		$parser->match_TopTemplate();
 
 		return self::$entities;

model/DB.php

@@ -135,6 +135,10 @@ class DB {
 	 * Set it to null to revert to the main database.
 	 */
 	public static function set_alternative_database_name($name = null) {
+		// Skip if CLI
+		if(Director::is_cli()) {
+			return;
+		}
 		if($name) {
 			if(!self::valid_alternative_database_name($name)) {
 				throw new InvalidArgumentException(sprintf(

model/DataQuery.php

@@ -333,7 +333,7 @@ class DataQuery {
 
 					if(!in_array($qualCol, $query->getSelect())) {
 						unset($newOrderby[$k]);
-						
+
 						$newOrderby["\"_SortColumn$i\""] = $dir;
 						$query->selectField($qualCol, "_SortColumn$i");
 

parsers/ShortcodeParser.php

@@ -4,27 +4,27 @@
  * It is a simple regex based parser that allows you to replace simple bbcode-like tags
  * within a HTMLText or HTMLVarchar field when rendered into a template. The API is inspired by and very similar to the
  * [Wordpress implementation](http://codex.wordpress.org/Shortcode_API) of shortcodes.
- *
+ * 
  * @see http://doc.silverstripe.org/framework/en/reference/shortcodes
  * @package framework
  * @subpackage misc
  */
 class ShortcodeParser extends Object {
-
+	
 	public function img_shortcode($attrs) {
 		return "<img src='".$attrs['src']."'>";
 	}
-
+	
 	protected static $instances = array();
-
+	
 	protected static $active_instance = 'default';
-
+	
 	// --------------------------------------------------------------------------------------------------------------
-
+	
 	protected $shortcodes = array();
-
+	
 	// --------------------------------------------------------------------------------------------------------------
-
+	
 	/**
 	 * Get the {@link ShortcodeParser} instance that is attached to a particular identifier.
 	 *
@@ -35,10 +35,10 @@ class ShortcodeParser extends Object {
 		if(!array_key_exists($identifier, self::$instances)) {
 			self::$instances[$identifier] = static::create();
 		}
-
+		
 		return self::$instances[$identifier];
 	}
-
+	
 	/**
 	 * Get the currently active/default {@link ShortcodeParser} instance.
 	 *
@@ -47,7 +47,7 @@ class ShortcodeParser extends Object {
 	public static function get_active() {
 		return static::get(self::$active_instance);
 	}
-
+	
 	/**
 	 * Set the identifier to use for the current active/default {@link ShortcodeParser} instance.
 	 *
@@ -56,9 +56,9 @@ class ShortcodeParser extends Object {
 	public static function set_active($identifier) {
 		self::$active_instance = (string) $identifier;
 	}
-
+	
 	// --------------------------------------------------------------------------------------------------------------
-
+	
 	/**
 	 * Register a shortcode, and attach it to a PHP callback.
 	 *
@@ -76,7 +76,7 @@ class ShortcodeParser extends Object {
 	public function register($shortcode, $callback) {
 		if(is_callable($callback)) $this->shortcodes[$shortcode] = $callback;
 	}
-
+	
 	/**
 	 * Check if a shortcode has been registered.
 	 *
@@ -86,7 +86,7 @@ class ShortcodeParser extends Object {
 	public function registered($shortcode) {
 		return array_key_exists($shortcode, $this->shortcodes);
 	}
-
+	
 	/**
 	 * Remove a specific registered shortcode.
 	 *
@@ -95,28 +95,64 @@ class ShortcodeParser extends Object {
 	public function unregister($shortcode) {
 		if($this->registered($shortcode)) unset($this->shortcodes[$shortcode]);
 	}
-
+	
 	/**
 	 * Remove all registered shortcodes.
 	 */
 	public function clear() {
 		$this->shortcodes = array();
 	}
-
+	
+	/**
+	 * Call a shortcode and return its replacement text
+	 * Returns false if the shortcode isn't registered
+	 */
 	public function callShortcode($tag, $attributes, $content, $extra = array()) {
-		if (!isset($this->shortcodes[$tag])) return false;
+		if (!$tag || !isset($this->shortcodes[$tag])) return false;
 		return call_user_func($this->shortcodes[$tag], $attributes, $content, $this, $tag, $extra);
 	}
+	
+	/**
+	 * Return the text to insert in place of a shoprtcode.
+	 * Behaviour in the case of missing shortcodes depends on the setting of ShortcodeParser::$error_behavior.
+	 * @param $tag A map containing the the following keys:
+	 *  - 'open': The name of the tag
+	 *  - 'attrs': Attributes of the tag
+	 *  - 'content': Content of the tag
+	 * @param $extra Extra-meta data
+	 * @param $isHTMLAllowed A boolean indicating whether it's okay to insert HTML tags into the result
+	 */
+	function getShortcodeReplacementText($tag, $extra = array(), $isHTMLAllowed = true) {
+		$content = $this->callShortcode($tag['open'], $tag['attrs'], $tag['content'], $extra);
+
+		// Missing tag
+		if ($content === false) {
+			if(ShortcodeParser::$error_behavior == ShortcodeParser::ERROR) {
+				user_error('Unknown shortcode tag '.$tag['open'], E_USER_ERRROR);
+			}
+			else if (self::$error_behavior == self::WARN && $isHTMLAllowed) {
+				$content = '<strong class="warning">'.$tag['text'].'</strong>';
+			}
+			else if(ShortcodeParser::$error_behavior == ShortcodeParser::STRIP) {
+				return '';
+			}
+			else {
+				return $tag['text'];
+			}
+		}
+
+		return $content;
+	}
 
 	// --------------------------------------------------------------------------------------------------------------
-
+	
 	protected function removeNode($node) {
 		$node->parentNode->removeChild($node);
 	}
-
+	
 	protected function insertAfter($new, $after) {
 		$parent = $after->parentNode; $next = $after->nextSibling;
-
+		
 		if ($next) {
 			$parent->insertBefore($new, $next);
 		}
@@ -124,7 +160,7 @@ class ShortcodeParser extends Object {
 			$parent->appendChild($new);
 		}
 	}
-
+	
 	protected function insertListAfter($new, $after) {
 		$doc = $after->ownerDocument; $parent = $after->parentNode; $next = $after->nextSibling;
 
@@ -132,8 +168,8 @@ class ShortcodeParser extends Object {
 			$imported = $doc->importNode($new->item($i), true);
 
 			if ($next) {
-				$parent->insertBefore($imported, $next);
-			}
+				$parent->insertBefore($imported, $next);	
+			} 
 			else {
 				$parent->appendChild($imported);
 			}
@@ -147,9 +183,9 @@ class ShortcodeParser extends Object {
 		'figure', 'footer', 'form', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'header', 'hgroup', 'ol', 'output', 'p',
 		'pre', 'section', 'table', 'ul'
 	);
-
+	
 	protected static $attrrx = '
-		([^\s\/\'"=,]+)       # Name
+		([^\s\/\'"=,]+)       # Name  
 		\s* = \s*
 		(?:
 			(?:\'([^\']+)\') | # Value surrounded by \'
@@ -157,7 +193,7 @@ class ShortcodeParser extends Object {
 			([^\s,\]]+)          # Bare value
 		)
 ';
-
+	
 	protected static function attrrx() {
 		return '/'.self::$attrrx.'/xS';
 	}
@@ -165,21 +201,21 @@ class ShortcodeParser extends Object {
 	protected static $tagrx = '
 		# HTML Tag
 		<(?<element>(?:"[^"]*"[\'"]*|\'[^\']*\'[\'"]*|[^\'">])+)>
-
+		 
 		| # Opening tag
-		(?<oesc>\[?)
-		\[
-			(?<open>\w+)
+		(?<oesc>\[?) 
+		\[ 
+			(?<open>\w+) 
 			[\s,]*
-			(?<attrs> (?: %s [\s,]*)* )
-		\/?\]
+			(?<attrs> (?: %s [\s,]*)* ) 
+		\/?\] 
 		(?<cesc1>\]?)
-
+		 
 		| # Closing tag
-		\[\/
-			(?<close>\w+)
-		\]
-		(?<cesc2>\]?)
+		\[\/ 
+			(?<close>\w+) 
+		\] 
+		(?<cesc2>\]?)  
 ';
 
 	protected static function tagrx() {
@@ -199,14 +235,15 @@ class ShortcodeParser extends Object {
 	 * of those tags
 	 *
 	 * Doesn't support nested shortcode tags
-	 *
+	 * 
 	 * @param string $content
 	 * @return array - The list of tags found. When using an open/close pair, only one item will be in the array,
 	 * with "content" set to the text between the tags
 	 */
 	protected function extractTags($content) {
 		$tags = array();
-
+		
+		// Step 1: perform basic regex scan of individual tags
 		if(preg_match_all(static::tagrx(), $content, $matches, PREG_SET_ORDER | PREG_OFFSET_CAPTURE)) {
 			foreach($matches as $match) {
 				// Ignore any elements
@@ -214,7 +251,7 @@ class ShortcodeParser extends Object {
 
 				// Pull the attributes out into a key/value hash
 				$attrs = array();
-
+				
 				if (!empty($match['attrs'][0])) {
 					preg_match_all(static::attrrx(), $match['attrs'][0], $attrmatches, PREG_SET_ORDER);
 
@@ -223,7 +260,7 @@ class ShortcodeParser extends Object {
 						$attrs[$name] = $value;
 				}
 				}
-
+				
 				// And store the indexes, tag details, etc
 				$tags[] = array(
 					'text' => $match[0][0],
@@ -236,14 +273,15 @@ class ShortcodeParser extends Object {
 					'escaped' => !empty($match['oesc'][0]) || !empty($match['cesc1'][0]) || !empty($match['cesc2'][0])
 				);
 			}
-			}
+		}
 
+		// Step 2: cluster open/close tag pairs into single entries
 		$i = count($tags);
 		while($i--) {
 			if(!empty($tags[$i]['close'])) {
 				// If the tag just before this one isn't the related opening tag, throw an error
 				$err = null;
-
+				
 				if ($i == 0) {
 					$err = 'Close tag "'.$tags[$i]['close'].'" is the first found tag, so has no related open tag';
 				}
@@ -255,10 +293,10 @@ class ShortcodeParser extends Object {
 					$err = 'Close tag "'.$tags[$i]['close'].'" doesn\'t match preceding open tag "'.
 							$tags[$i-1]['open'].'"';
 				}
-
+		
 				if($err) {
-					if(self::$error_behavior == self::ERROR) user_error($err, E_USER_ERRROR);
-				}
+					if(self::$error_behavior == self::ERROR) user_error($err, E_USER_ERRROR);	
+				} 
 				else {
 					if ($tags[$i]['escaped']) {
 						if (!$tags[$i-1]['escaped']) {
@@ -272,12 +310,23 @@ class ShortcodeParser extends Object {
 							$tags[$i-1]['escaped'] = false;
 						}
 					}
-
+					
 					// Otherwise, grab content between tags, save in opening tag & delete the closing one
 					$tags[$i-1]['text'] = substr($content, $tags[$i-1]['s'], $tags[$i]['e'] - $tags[$i-1]['s']);
 					$tags[$i-1]['content'] = substr($content, $tags[$i-1]['e'], $tags[$i]['s'] - $tags[$i-1]['e']);
 					$tags[$i-1]['e'] = $tags[$i]['e'];
+					
+					unset($tags[$i]);
+				}
+			}
+		}
 
+		// Step 3: remove any tags that don't have handlers registered
+		// Only do this if self::$error_behavior == self::LEAVE
+		// This is optional but speeds things up.
+		if(self::$error_behavior == self::LEAVE) {
+			foreach($tags as $i => $tag) {
+				if(empty($this->shortcodes[$tag['open']])) {
 					unset($tags[$i]);
 				}
 			}
@@ -311,7 +360,7 @@ class ShortcodeParser extends Object {
 			else {
 				$str = $generator($i, $tags[$i]) . $tail . $str;
 			}
-
+			
 			$li = $tags[$i]['s'];
 		}
 
@@ -337,27 +386,14 @@ class ShortcodeParser extends Object {
 
 			if($tags) {
 				$node->nodeValue = $this->replaceTagsWithText($node->nodeValue, $tags,
-					function($idx, $tag) use ($parser, $extra){
-					$content = $parser->callShortcode($tag['open'], $tag['attrs'], $tag['content'], $extra);
-
-					if ($content === false) {
-						if(ShortcodeParser::$error_behavior == ShortcodeParser::ERROR) {
-							user_error('Unknown shortcode tag '.$tag['open'], E_USER_ERRROR);
-						}
-						else if(ShortcodeParser::$error_behavior == ShortcodeParser::STRIP) {
-							return '';
-						}
-						else {
-							return $tag['text'];
-						}
+					function($idx, $tag) use ($parser, $extra) {
+						return $parser->getShortcodeReplacementText($tag, $extra, false);
 					}
-
-		return $content;
-				});
-	}
+				);
+			}
 		}
 	}
-
+	
 	/**
 	 * Replace the element-scoped tags with markers
 	 *
@@ -368,18 +404,18 @@ class ShortcodeParser extends Object {
 
 		if($tags) {
 			$markerClass = self::$marker_class;
-
+		
 			$content = $this->replaceTagsWithText($content, $tags, function($idx, $tag) use ($markerClass) {
 				return '<img class="'.$markerClass.'" data-tagid="'.$idx.'" />';
 			});
-				}
-
+		}
+		
 		return array($content, $tags);
-			}
+	}
 
 	protected function findParentsForMarkers($nodes) {
 		$parents = array();
-
+		
 		foreach($nodes as $node) {
 			$parent = $node;
 
@@ -392,34 +428,34 @@ class ShortcodeParser extends Object {
 			$node->setAttribute('data-parentid', count($parents));
 			$parents[] = $parent;
 	}
-
+	
 		return $parents;
 }
-
+	
 	const BEFORE = 'before';
 	const AFTER = 'after';
 	const SPLIT = 'split';
 	const INLINE = 'inline';
-
+	
 	/**
 	 * Given a node with represents a shortcode marker and a location string, mutates the DOM to put the
 	 * marker in the compliant location
-	 *
+	 * 
 	 * For shortcodes inserted BEFORE, that location is just before the block container that
 	 * the marker is in
-	 *
+	 * 
 	 * For shortcodes inserted AFTER, that location is just after the block container that
 	 * the marker is in
-	 *
+	 * 
 	 * For shortcodes inserted SPLIT, that location is where the marker is, but the DOM
 	 * is split around it up to the block container the marker is in - for instance,
-	 *
+	 * 
 	 *   <p>A<span>B<marker />C</span>D</p>
-	 *
-	 * becomes
-	 *
+	 * 
+	 * becomes 
+	 * 
 	 *   <p>A<span>B</span></p><marker /><p><span>C</span>D</p>
-	 *
+	 * 
 	 * For shortcodes inserted INLINE, no modification is needed (but in that case the shortcode handler needs to
 	 * generate only inline blocks)
 	 *
@@ -458,7 +494,7 @@ class ShortcodeParser extends Object {
 			if(in_array(strtolower($node->tagName), self::$block_level_elements)) {
 				user_error(
 					'Requested to insert block tag '.$node->tagName.
-					' inline - probably this will break HTML compliance',
+					' inline - probably this will break HTML compliance', 
 					E_USER_WARNING
 				);
 			}
@@ -472,35 +508,19 @@ class ShortcodeParser extends Object {
 	/**
 	 * Given a node with represents a shortcode marker and some information about the shortcode, call the
 	 * shortcode handler & replace the marker with the actual content
-	 *
+	 * 
 	 * @param DOMElement $node
 	 * @param array $tag
 	 */
 	protected function replaceMarkerWithContent($node, $tag) {
-		$content = false;
-		if($tag['open']) $content = $this->callShortcode($tag['open'], $tag['attrs'], $tag['content']);
-
-		if ($content === false) {
-			if(self::$error_behavior == self::ERROR) {
-				user_error('Unknown shortcode tag '.$tag['open'], E_USER_ERRROR);
-			}
-			if (self::$error_behavior == self::WARN) {
-				$content = '<strong class="warning">'.$tag['text'].'</strong>';
-			}
-			else if (self::$error_behavior == self::LEAVE) {
-				$content = $tag['text'];
-			}
-			else {
-				// self::$error_behavior == self::STRIP - NOP
-			}
-		}
-
+		$content = $this->getShortcodeReplacementText($tag);
+		
 		if ($content) {
 			$parsed = Injector::inst()->create('HTMLValue', $content);
 			$body = $parsed->getBody();
 			if ($body) $this->insertListAfter($body->childNodes, $node);
 		}
-
+		
 		$this->removeNode($node);
 	}
 
@@ -545,11 +565,11 @@ class ShortcodeParser extends Object {
 		foreach($shortcodes as $shortcode) {
 			$tag = $tags[$shortcode->getAttribute('data-tagid')];
 			$parent = $parents[$shortcode->getAttribute('data-parentid')];
-
+			
 			$class = null;
-			if(!empty($tag['attrs']['location'])) $class = $tag['attrs']['location'];
+			if(!empty($tag['attrs']['location'])) $class = $tag['attrs']['location']; 
 			else if(!empty($tag['attrs']['class'])) $class = $tag['attrs']['class'];
-
+			
 			$location = self::INLINE;
 			if($class == 'left' || $class == 'right') $location = self::BEFORE;
 			if($class == 'center' || $class == 'leftALone') $location = self::SPLIT;
@@ -567,8 +587,21 @@ class ShortcodeParser extends Object {
 			$this->replaceMarkerWithContent($shortcode, $tag);
 		}
 
-		return $htmlvalue->getContent();
+		$content = $htmlvalue->getContent();
+
+		// Clean up any marker classes left over, for example, those injected into <script> tags
+		$parser = $this;
+		$content = preg_replace_callback(
+			// Not a general-case parser; assumes that the HTML generated in replaceElementTagsWithMarkers()
+			// hasn't been heavily modified
+			'/<img[^>]+class="'.preg_quote(self::$marker_class).'"[^>]+data-tagid="([^"]+)"[^>]+>/i',
+			function ($matches) use ($tags, $parser) {
+				$tag = $tags[$matches[1]];
+				return $parser->getShortcodeReplacementText($tag);
+			},
+			$content
+		);
+
+		return $content;
 	}
-
-
 }

security/ChangePasswordForm.php

@@ -5,7 +5,7 @@
  * @subpackage security
  */
 class ChangePasswordForm extends Form {
-
+	
 	/**
 	 * Constructor
 	 *
@@ -28,7 +28,7 @@ class ChangePasswordForm extends Form {
 
 		if(!$fields) {
 			$fields = new FieldList();
-
+			
 			// Security/changepassword?h=XXX redirects to Security/changepassword
 			// without GET parameter to avoid potential HTTP referer leakage.
 			// In this case, a user is not logged in, and no 'old password' should be necessary.
@@ -65,7 +65,7 @@ class ChangePasswordForm extends Form {
 			if(empty($data['OldPassword']) || !$member->checkPassword($data['OldPassword'])->valid()) {
 				$this->clearMessage();
 				$this->sessionMessage(
-					_t('Member.ERRORPASSWORDNOTMATCH', "Your current password does not match, please try again"),
+					_t('Member.ERRORPASSWORDNOTMATCH', "Your current password does not match, please try again"), 
 					"bad"
 				);
 				// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.
@@ -99,7 +99,7 @@ class ChangePasswordForm extends Form {
 			$isValid = $member->changePassword($data['NewPassword1']);
 			if($isValid->valid()) {
 				$member->logIn();
-
+				
 				// TODO Add confirmation message to login redirect
 				Session::clear('AutoLoginHash');
 
@@ -127,11 +127,12 @@ class ChangePasswordForm extends Form {
 				$this->clearMessage();
 				$this->sessionMessage(
 					_t(
-						'Member.INVALIDNEWPASSWORD',
+						'Member.INVALIDNEWPASSWORD', 
 						"We couldn't accept that password: {password}",
-						array('password' => nl2br("\n".$isValid->starredList()))
-					),
-					"bad"
+						array('password' => nl2br("\n".Convert::raw2xml($isValid->starredList())))
+					), 
+					"bad",
+					false
 				);
 
 				// redirect back to the form, instead of using redirectBack() which could send the user elsewhere.

tasks/i18nTextCollectorTask.php

@@ -4,9 +4,9 @@
  * @subpackage tasks
  */
 class i18nTextCollectorTask extends BuildTask {
-
+	
 	protected $title = "i18n Textcollector Task";
-
+	
 	protected $description = "
 		Traverses through files in order to collect the 'entity master tables'
 		stored in each module.
@@ -17,27 +17,66 @@ class i18nTextCollectorTask extends BuildTask {
 		- module: One or more modules to limit collection (comma-separated)
 		- merge: Merge new strings with existing ones already defined in language files (default: FALSE)
 	";
-
+	
 	public function init() {
 		parent::init();
-
+		
 		$canAccess = (Director::isDev() || Director::is_cli() || Permission::check("ADMIN"));
-		if(!$canAccess) return Security::permissionFailure($this);
+		if(!$canAccess) {
+			return Security::permissionFailure($this);
+		}
 	}
-
+	
 	/**
 	 * This is the main method to build the master string tables with the original strings.
 	 * It will search for existent modules that use the i18n feature, parse the _t() calls
 	 * and write the resultant files in the lang folder of each module.
-	 *
+	 * 
 	 * @uses DataObject->collectI18nStatics()
-	 */
+	 * 
+	 * @param SS_HTTPRequest $request
+	 */	
 	public function run($request) {
 		increase_time_limit_to();
-		$c = new i18nTextCollector($request->getVar('locale'));
-		$writer = $request->getVar('writer');
-		if($writer) $c->setWriter(new $writer());
-		$restrictModules = ($request->getVar('module')) ? explode(',', $request->getVar('module')) : null;
-		return $c->run($restrictModules, (bool)$request->getVar('merge'));
+		$collector = i18nTextCollector::create($request->getVar('locale'));
+		
+		$merge = $this->getIsMerge($request);
+		
+		// Custom writer
+		$writerName = $request->getVar('writer');
+		if($writerName) {
+			$writer = Injector::inst()->get($writerName);
+			$collector->setWriter($writer);
+		}
+		
+		// Get restrictions
+		$restrictModules = ($request->getVar('module'))
+			? explode(',', $request->getVar('module'))
+			: null;
+		
+		$collector->run($restrictModules, $merge);
+		
+		Debug::message(__CLASS__ . " completed!", false);
+	}
+	
+	/**
+	 * Check if we should merge
+	 * 
+	 * @param SS_HTTPRequest $request
+	 */
+	protected function getIsMerge($request) {
+		$merge = $request->getVar('merge');
+		
+		// Default to false if not given
+		if(!isset($merge)) {
+			Deprecation::notice(
+				"4.0",
+				"merge will be enabled by default in 4.0. Please use merge=false if you do not want to merge."
+			);
+			return false;
+		}
+
+		// merge=0 or merge=false will disable merge
+		return !in_array($merge, array('0', 'false'));
 	}
 }

tests/i18n/_fakewebroot/i18nnonstandardmodule/phpfile.php

@@ -0,0 +1 @@
+<?php

tests/i18n/_fakewebroot/i18nnonstandardmodule/template.ss

@@ -0,0 +1 @@
+<p></p>

tests/i18n/_fakewebroot/i18nnonstandardmodule/tests/NothingInHere.php

@@ -0,0 +1 @@
+<?php

tests/i18n/_fakewebroot/i18ntestmodule/tests/DontLook.php

@@ -0,0 +1 @@
+<?php

tests/i18n/_fakewebroot/notmodule/noop.php

@@ -0,0 +1,2 @@
+<?php
+

tests/i18n/i18nTextCollectorTest.php

@@ -4,25 +4,25 @@
  * @subpackage tests
  */
 class i18nTextCollectorTest extends SapphireTest {
-
+	
 	/**
 	 * @var string $tmpBasePath Used to write language files.
 	 * We don't want to store them inside framework (or in any web-accessible place)
 	 * in case something goes wrong with the file parsing.
 	 */
 	protected $alternateBaseSavePath;
-
+	
 	/**
 	 * @var string $alternateBasePath Fake webroot with a single module
 	 * /i18ntestmodule which contains some files with _t() calls.
 	 */
 	protected $alternateBasePath;
-
+	
 	protected $manifest;
-
+	
 	public function setUp() {
 		parent::setUp();
-
+		
 		$this->alternateBasePath = $this->getCurrentAbsolutePath() . "/_fakewebroot";
 		$this->alternateBaseSavePath = TEMP_FOLDER . '/i18nTextCollectorTest_webroot';
 		Filesystem::makeFolder($this->alternateBaseSavePath);
@@ -32,12 +32,12 @@ class i18nTextCollectorTest extends SapphireTest {
 		$this->manifest = new SS_ClassManifest(
 			$this->alternateBasePath, false, true, false
 		);
-
+		
 		$manifest = new SS_TemplateManifest($this->alternateBasePath, null, false, true);
 		$manifest->regenerate(false);
 		SS_TemplateLoader::instance()->pushManifest($manifest);
 	}
-
+	
 	public function tearDown() {
 		SS_TemplateLoader::instance()->popManifest();
 		parent::tearDown();
@@ -58,7 +58,7 @@ _t(
 
 _t(
 'Test.CONCATENATED2',
-"Line \"4\" and " .
+"Line \"4\" and " . 
 "Line 5");
 PHP;
 		$this->assertEquals(
@@ -120,7 +120,7 @@ SS;
 				'Test.DOUBLEQUOTE' => array("Double Quote and Spaces")
 			)
 		);
-
+		
 		$html = <<<SS
 <% _t("Test.NOSEMICOLON","No Semicolon") %>
 SS;
@@ -166,7 +166,7 @@ SS;
 <% _t(
 	'Test.PRIOANDCOMMENT',
 	" Prio and Value with 'Single Quotes'",
-
+	
 	"Comment with 'Single Quotes'"
 ) %>
 SS;
@@ -181,7 +181,7 @@ SS;
 
 	public function testCollectFromCodeSimple() {
 		$c = new i18nTextCollector();
-
+			
 		$php = <<<PHP
 _t('Test.SINGLEQUOTE','Single Quote');
 PHP;
@@ -191,7 +191,7 @@ PHP;
 				'Test.SINGLEQUOTE' => array('Single Quote')
 			)
 		);
-
+		
 		$php = <<<PHP
 _t(  "Test.DOUBLEQUOTE", "Double Quote and Spaces"   );
 PHP;
@@ -202,7 +202,7 @@ PHP;
 			)
 		);
 	}
-
+	
 	public function testCollectFromCodeAdvanced() {
 		$c = new i18nTextCollector();
 
@@ -218,12 +218,12 @@ PHP;
 				'Test.NEWLINES' => array("New Lines")
 			)
 		);
-
+		
 		$php = <<<PHP
 _t(
 	'Test.PRIOANDCOMMENT',
 	' Value with "Double Quotes"',
-
+	
 	'Comment with "Double Quotes"'
 );
 PHP;
@@ -233,12 +233,12 @@ PHP;
 				'Test.PRIOANDCOMMENT' => array(' Value with "Double Quotes"','Comment with "Double Quotes"')
 			)
 		);
-
+		
 		$php = <<<PHP
 _t(
 	'Test.PRIOANDCOMMENT',
 	" Value with 'Single Quotes'",
-
+	
 	"Comment with 'Single Quotes'"
 );
 PHP;
@@ -248,7 +248,7 @@ PHP;
 				'Test.PRIOANDCOMMENT' => array(" Value with 'Single Quotes'","Comment with 'Single Quotes'")
 			)
 		);
-
+		
 		$php = <<<PHP
 _t(
 	'Test.PRIOANDCOMMENT',
@@ -261,7 +261,7 @@ PHP;
 				'Test.PRIOANDCOMMENT' => array("Value with 'Escaped Single Quotes'")
 			)
 		);
-
+	
 		$php = <<<PHP
 _t(
 	'Test.PRIOANDCOMMENT',
@@ -275,8 +275,8 @@ PHP;
 			)
 		);
 	}
-
-
+	
+	
 	public function testNewlinesInEntityValues() {
 		$c = new i18nTextCollector();
 
@@ -364,24 +364,24 @@ PHP;
 	 */
 	public function testPhpWriterLangArrayCodeForEntity() {
 		$c = new i18nTextCollector_Writer_Php();
-
+		
 		$this->assertEquals(
 			$c->langArrayCodeForEntitySpec('Test.SIMPLE', array('Simple Value'), 'en_US'),
 			"\$lang['en_US']['Test']['SIMPLE'] = 'Simple Value';" . PHP_EOL
 		);
-
+		
 		$this->assertEquals(
 			// single quotes should be properly escaped by the parser already
 			$c->langArrayCodeForEntitySpec('Test.ESCAPEDSINGLEQUOTES',
 				array("Value with 'Escaped Single Quotes'"), 'en_US'),
 			"\$lang['en_US']['Test']['ESCAPEDSINGLEQUOTES'] = 'Value with \'Escaped Single Quotes\'';" . PHP_EOL
 		);
-
+		
 		$this->assertEquals(
 			$c->langArrayCodeForEntitySpec('Test.DOUBLEQUOTES', array('Value with "Double Quotes"'), 'en_US'),
 			"\$lang['en_US']['Test']['DOUBLEQUOTES'] = 'Value with \"Double Quotes\"';" . PHP_EOL
 		);
-
+		
 		$php = <<<PHP
 \$lang['en_US']['Test']['PRIOANDCOMMENT'] = array (
   0 => 'Value with \'Single Quotes\'',
@@ -394,7 +394,7 @@ PHP;
 				array("Value with 'Single Quotes'","Comment with 'Single Quotes'"), 'en_US'),
 			$php
 		);
-
+		
 		$php = <<<PHP
 \$lang['en_US']['Test']['PRIOANDCOMMENT'] = array (
   0 => 'Value with "Double Quotes"',
@@ -434,21 +434,19 @@ de:
 YAML;
 		$this->assertEquals($yaml, Convert::nl2os($writer->getYaml($entities, 'de')));
 	}
-
+	
 	public function testCollectFromIncludedTemplates() {
 		$c = new i18nTextCollector();
-
+		
 		$templateFilePath = $this->alternateBasePath . '/i18ntestmodule/templates/Layout/i18nTestModule.ss';
 		$html = file_get_contents($templateFilePath);
 		$matches = $c->collectFromTemplate($html, 'mymodule', 'RandomNamespace');
 
-		/*
-		$this->assertArrayHasKey('i18nTestModule.ss.LAYOUTTEMPLATENONAMESPACE', $matches);
+		$this->assertArrayHasKey('RandomNamespace.LAYOUTTEMPLATENONAMESPACE', $matches);
 		$this->assertEquals(
-			$matches['i18nTestModule.ss.LAYOUTTEMPLATENONAMESPACE'],
+			$matches['RandomNamespace.LAYOUTTEMPLATENONAMESPACE'],
 			array('Layout Template no namespace')
 		);
-		*/
 		$this->assertArrayHasKey('RandomNamespace.SPRINTFNONAMESPACE', $matches);
 		$this->assertEquals(
 			$matches['RandomNamespace.SPRINTFNONAMESPACE'],
@@ -464,98 +462,70 @@ YAML;
 			$matches['i18nTestModule.SPRINTFNAMESPACE'],
 			array('My replacement: %s')
 		);
-		$this->assertArrayHasKey('i18nTestModule.WITHNAMESPACE', $matches);
-		$this->assertEquals(
-			$matches['i18nTestModule.WITHNAMESPACE'],
-			array('Include Entity with Namespace')
-		);
-		$this->assertArrayHasKey('i18nTestModuleInclude.ss.NONAMESPACE', $matches);
-		$this->assertEquals(
-			$matches['i18nTestModuleInclude.ss.NONAMESPACE'],
-			array('Include Entity without Namespace')
-		);
-		$this->assertArrayHasKey('i18nTestModuleInclude.ss.SPRINTFINCLUDENAMESPACE', $matches);
-		$this->assertEquals(
-			$matches['i18nTestModuleInclude.ss.SPRINTFINCLUDENAMESPACE'],
-			array('My include replacement: %s')
-		);
-		$this->assertArrayHasKey('i18nTestModuleInclude.ss.SPRINTFINCLUDENONAMESPACE', $matches);
-		$this->assertEquals(
-			$matches['i18nTestModuleInclude.ss.SPRINTFINCLUDENONAMESPACE'],
-			array('My include replacement no namespace: %s')
-		);
+		
+		// Includes should not automatically inject translations into parent templates
+		$this->assertArrayNotHasKey('i18nTestModule.WITHNAMESPACE', $matches);
+		$this->assertArrayNotHasKey('i18nTestModuleInclude.ss.NONAMESPACE', $matches);
+		$this->assertArrayNotHasKey('i18nTestModuleInclude.ss.SPRINTFINCLUDENAMESPACE', $matches);
+		$this->assertArrayNotHasKey('i18nTestModuleInclude.ss.SPRINTFINCLUDENONAMESPACE', $matches);
 	}
-
+	
 	public function testCollectFromThemesTemplates() {
 		$c = new i18nTextCollector();
-
-		$theme = Config::inst()->get('SSViewer', 'theme');
 		Config::inst()->update('SSViewer', 'theme', 'testtheme1');
-
-		$templateFilePath = $this->alternateBasePath . '/themes/testtheme1/templates/Layout/i18nTestTheme1.ss';
-		$html = file_get_contents($templateFilePath);
-		$matches = $c->collectFromTemplate($html, 'themes/testtheme1', 'i18nTestTheme1.ss');
+		
+		// Collect from layout
+		$layoutFilePath = $this->alternateBasePath . '/themes/testtheme1/templates/Layout/i18nTestTheme1.ss';
+		$layoutHTML = file_get_contents($layoutFilePath);
+		$layoutMatches = $c->collectFromTemplate($layoutHTML, 'themes/testtheme1', 'i18nTestTheme1.ss');
+		
 		// all entities from i18nTestTheme1.ss
 		$this->assertEquals(
-			$matches['i18nTestTheme1.LAYOUTTEMPLATE'],
-			array('Theme1 Layout Template')
+			array(
+				'i18nTestTheme1.LAYOUTTEMPLATE'
+					=> array('Theme1 Layout Template'),
+				'i18nTestTheme1.SPRINTFNAMESPACE'
+					=> array('Theme1 My replacement: %s'),
+				'i18nTestTheme1.ss.LAYOUTTEMPLATENONAMESPACE'
+					=> array('Theme1 Layout Template no namespace'),
+				'i18nTestTheme1.ss.SPRINTFNONAMESPACE'
+					=> array('Theme1 My replacement no namespace: %s'),
+			),
+			$layoutMatches
 		);
-
-		$this->assertArrayHasKey('i18nTestTheme1.ss.LAYOUTTEMPLATENONAMESPACE', $matches);
-		$this->assertEquals(
-			$matches['i18nTestTheme1.ss.LAYOUTTEMPLATENONAMESPACE'],
-			array('Theme1 Layout Template no namespace')
-		);
-
-		$this->assertEquals(
-			$matches['i18nTestTheme1.SPRINTFNAMESPACE'],
-			array('Theme1 My replacement: %s')
-		);
-
-		$this->assertArrayHasKey('i18nTestTheme1.ss.SPRINTFNONAMESPACE', $matches);
-		$this->assertEquals(
-			$matches['i18nTestTheme1.ss.SPRINTFNONAMESPACE'],
-			array('Theme1 My replacement no namespace: %s')
-		);
-
+		
+		// Collect from include
+		$includeFilePath = $this->alternateBasePath . '/themes/testtheme1/templates/Includes/i18nTestTheme1Include.ss';
+		$includeHTML = file_get_contents($includeFilePath);
+		$includeMatches = $c->collectFromTemplate($includeHTML, 'themes/testtheme1', 'i18nTestTheme1Include.ss');
+		
 		// all entities from i18nTestTheme1Include.ss
 		$this->assertEquals(
-			$matches['i18nTestTheme1Include.WITHNAMESPACE'],
-			array('Theme1 Include Entity with Namespace')
+			array(
+				'i18nTestTheme1Include.SPRINTFINCLUDENAMESPACE'
+					=> array('Theme1 My include replacement: %s'),
+				'i18nTestTheme1Include.WITHNAMESPACE'
+					=> array('Theme1 Include Entity with Namespace'),
+				'i18nTestTheme1Include.ss.NONAMESPACE'
+					=> array('Theme1 Include Entity without Namespace'),
+				'i18nTestTheme1Include.ss.SPRINTFINCLUDENONAMESPACE'
+					=> array('Theme1 My include replacement no namespace: %s')
+			),
+			$includeMatches
 		);
-
-		$this->assertArrayHasKey('i18nTestTheme1Include.ss.NONAMESPACE', $matches);
-		$this->assertEquals(
-			$matches['i18nTestTheme1Include.ss.NONAMESPACE'],
-			array('Theme1 Include Entity without Namespace')
-		);
-
-
-		$this->assertEquals(
-			$matches['i18nTestTheme1Include.SPRINTFINCLUDENAMESPACE'],
-			array('Theme1 My include replacement: %s')
-		);
-
-		$this->assertArrayHasKey('i18nTestTheme1Include.ss.SPRINTFINCLUDENONAMESPACE', $matches);
-		$this->assertEquals(
-			$matches['i18nTestTheme1Include.ss.SPRINTFINCLUDENONAMESPACE'],
-			array('Theme1 My include replacement no namespace: %s')
-		);
-
-		Config::inst()->update('SSViewer', 'theme', $theme);
 	}
 
 	public function testCollectMergesWithExisting() {
 		$defaultlocal = i18n::default_locale();
 		$local = i18n::get_locale();
-		i18n::set_locale('en_US');
+		i18n::set_locale('en_US'); 
 		i18n::set_default_locale('en_US');
 
 		$c = new i18nTextCollector();
 		$c->setWriter(new i18nTextCollector_Writer_Php());
 		$c->basePath = $this->alternateBasePath;
 		$c->baseSavePath = $this->alternateBaseSavePath;
-
+		
 		$entitiesByModule = $c->collect(null, true /* merge */);
 		$this->assertArrayHasKey(
 			'i18nTestModule.ENTITY',
@@ -568,7 +538,7 @@ YAML;
 			'Adds new entities'
 		);
 	}
-
+	
 	public function testCollectFromFilesystemAndWriteMasterTables() {
 		$defaultlocal = i18n::default_locale();
 		$local = i18n::get_locale();
@@ -579,16 +549,16 @@ YAML;
 		$c->setWriter(new i18nTextCollector_Writer_Php());
 		$c->basePath = $this->alternateBasePath;
 		$c->baseSavePath = $this->alternateBaseSavePath;
-
+		
 		$c->run();
-
+		
 		// i18ntestmodule
 		$moduleLangFile = "{$this->alternateBaseSavePath}/i18ntestmodule/lang/" . $c->getDefaultLocale() . '.php';
 		$this->assertTrue(
 			file_exists($moduleLangFile),
 			'Master language file can be written to modules /lang folder'
 		);
-
+		
 		$moduleLangFileContent = file_get_contents($moduleLangFile);
 		$this->assertContains(
 			"\$lang['en']['i18nTestModule']['ADDITION'] = 'Addition';",
@@ -617,7 +587,7 @@ YAML;
 			"\$lang['en']['i18nTestModuleInclude.ss']['NONAMESPACE'] = 'Include Entity without Namespace';",
 			$moduleLangFileContent
 		);
-
+		
 		// i18nothermodule
 		$otherModuleLangFile = "{$this->alternateBaseSavePath}/i18nothermodule/lang/" . $c->getDefaultLocale() . '.php';
 		$this->assertTrue(
@@ -633,7 +603,7 @@ YAML;
 			"\$lang['en']['i18nOtherModule']['MAINTEMPLATE'] = 'Main Template Other Module';",
 			$otherModuleLangFileContent
 		);
-
+		
 		// testtheme1
 		$theme1LangFile = "{$this->alternateBaseSavePath}/themes/testtheme1/lang/" . $c->getDefaultLocale() . '.php';
 		$this->assertTrue(
@@ -661,7 +631,7 @@ YAML;
 			"\$lang['en']['i18nTestTheme1.ss']['SPRINTFNONAMESPACE'] = 'Theme1 My replacement no namespace: %s';",
 			$theme1LangFileContent
 		);
-
+		
 		$this->assertContains(
 			"\$lang['en']['i18nTestTheme1Include']['SPRINTFINCLUDENAMESPACE'] = 'Theme1 My include replacement: %s';",
 			$theme1LangFileContent
@@ -679,7 +649,7 @@ YAML;
 				. " 'Theme1 My include replacement no namespace: %s';",
 			$theme1LangFileContent
 		);
-
+		
 		// testtheme2
 		$theme2LangFile = "{$this->alternateBaseSavePath}/themes/testtheme2/lang/" . $c->getDefaultLocale() . '.php';
 		$this->assertTrue(
@@ -695,7 +665,7 @@ YAML;
 		i18n::set_locale($local);  //set the locale to the US locale expected in the asserts
 		i18n::set_default_locale($defaultlocal);
 	}
-
+	
 	public function testCollectFromEntityProvidersInCustomObject() {
 		$c = new i18nTextCollector();
 
@@ -713,5 +683,164 @@ YAML;
 			$matches['i18nTextCollectorTestMyObject.SINGULARNAME'][0]
 		);
 	}
+	
+	/**
+	 * Test that duplicate keys are resolved to the appropriate modules
+	 */
+	public function testResolveDuplicates() {
+		$collector = new i18nTextCollectorTest_Collector();
+		
+		// Dummy data as collected
+		$data1 = array(
+			'framework' => array(
+				'DataObject.PLURALNAME' => array('Data Objects'),
+				'DataObject.SINGULARNAME' => array('Data Object')
+			),
+			'mymodule' => array(
+				'DataObject.PLURALNAME' => array('Ignored String'),
+				'DataObject.STREETNAME' => array('Shortland Street')
+			)
+		);
+		$expected = array(
+			'framework' => array(
+				'DataObject.PLURALNAME' => array('Data Objects'),
+				// Because DataObject is in framework module
+				'DataObject.SINGULARNAME' => array('Data Object')
+			),
+			'mymodule' => array(
+				// Because this key doesn't exist in framework strings
+				'DataObject.STREETNAME' => array('Shortland Street')
+			)
+		);
+		
+		$resolved = $collector->resolveDuplicateConflicts_Test($data1);
+		$this->assertEquals($expected, $resolved);
+		
+		// Test getConflicts
+		$data2 = array(
+			'module1' => array(
+				'DataObject.ONE' => array('One'),
+				'DataObject.TWO' => array('Two'),
+				'DataObject.THREE' => array('Three'),
+			),
+			'module2' => array(
+				'DataObject.THREE' => array('Three'),
+			),
+			'module3' => array(
+				'DataObject.TWO' => array('Two'),
+				'DataObject.THREE' => array('Three'),
+			)
+		);
+		$conflictsA = $collector->getConflicts_Test($data2);
+		sort($conflictsA);
+		$this->assertEquals(
+			array('DataObject.THREE', 'DataObject.TWO'),
+			$conflictsA
+		);
+		
+		// Removing module3 should remove a conflict
+		unset($data2['module3']);
+		$conflictsB = $collector->getConflicts_Test($data2);
+		$this->assertEquals(
+			array('DataObject.THREE'),
+			$conflictsB
+		);
+	}
+	
+	/**
+	 * Test ability for textcollector to detect modules
+	 */
+	public function testModuleDetection() {
+		$collector = new i18nTextCollectorTest_Collector();
+		$modules = $collector->getModules_Test($this->alternateBasePath);
+		$this->assertEquals(
+			array(
+				'i18nnonstandardmodule',
+				'i18nothermodule',
+				'i18ntestmodule',
+				'themes/testtheme1',
+				'themes/testtheme2'
+			),
+			$modules
+		);
+	}
+	
+	/**
+	 * Test that text collector can detect module file lists properly
+	 */
+	public function testModuleFileList() {
+		$collector = new i18nTextCollectorTest_Collector();
+		$collector->basePath = $this->alternateBasePath;
+		$collector->baseSavePath = $this->alternateBaseSavePath;
+		
+		// Non-standard modules can't be safely filtered, so just index everything
+		$nonStandardFiles = $collector->getFileListForModule_Test('i18nnonstandardmodule');
+		$nonStandardRoot = $this->alternateBasePath . '/i18nnonstandardmodule';
+		$this->assertEquals(3, count($nonStandardFiles));
+		$this->assertArrayHasKey("{$nonStandardRoot}/_config.php", $nonStandardFiles);
+		$this->assertArrayHasKey("{$nonStandardRoot}/phpfile.php", $nonStandardFiles);
+		$this->assertArrayHasKey("{$nonStandardRoot}/template.ss", $nonStandardFiles);
+
+		// Normal module should have predictable dir structure
+		$testFiles = $collector->getFileListForModule_Test('i18ntestmodule');
+		$testRoot = $this->alternateBasePath . '/i18ntestmodule';
+		$this->assertEquals(6, count($testFiles));
+		// Code in code folder is detected
+		$this->assertArrayHasKey("{$testRoot}/code/i18nTestModule.php", $testFiles);
+		$this->assertArrayHasKey("{$testRoot}/code/subfolder/_config.php", $testFiles);
+		$this->assertArrayHasKey("{$testRoot}/code/subfolder/i18nTestSubModule.php", $testFiles);
+		// Templates in templates folder is detected
+		$this->assertArrayHasKey("{$testRoot}/templates/Includes/i18nTestModuleInclude.ss", $testFiles);
+		$this->assertArrayHasKey("{$testRoot}/templates/Layout/i18nTestModule.ss", $testFiles);
+		$this->assertArrayHasKey("{$testRoot}/templates/i18nTestModule.ss", $testFiles);
+		
+		// Standard modules with code in odd places should only have code in those directories detected
+		$otherFiles = $collector->getFileListForModule_Test('i18nothermodule');
+		$otherRoot = $this->alternateBasePath . '/i18nothermodule';
+		$this->assertEquals(3, count($otherFiles));
+		// Only detect well-behaved files
+		$this->assertArrayHasKey("{$otherRoot}/code/i18nOtherModule.php", $otherFiles);
+		$this->assertArrayHasKey("{$otherRoot}/code/i18nTestModuleDecorator.php", $otherFiles);
+		$this->assertArrayHasKey("{$otherRoot}/templates/i18nOtherModule.ss", $otherFiles);
+		
+		// Themes should detect all ss files only
+		$theme1Files = $collector->getFileListForModule_Test('themes/testtheme1');
+		$theme1Root = $this->alternateBasePath . '/themes/testtheme1/templates';
+		$this->assertEquals(3, count($theme1Files));
+		// Find only ss files
+		$this->assertArrayHasKey("{$theme1Root}/Includes/i18nTestTheme1Include.ss", $theme1Files);
+		$this->assertArrayHasKey("{$theme1Root}/Layout/i18nTestTheme1.ss", $theme1Files);
+		$this->assertArrayHasKey("{$theme1Root}/i18nTestTheme1Main.ss", $theme1Files);
+		
+		// Only 1 file here
+		$theme2Files = $collector->getFileListForModule_Test('themes/testtheme2');
+		$this->assertEquals(1, count($theme2Files));
+		$this->assertArrayHasKey(
+			$this->alternateBasePath . '/themes/testtheme2/templates/i18nTestTheme2.ss',
+			$theme2Files
+		);
+	}
+}
+
+
+/**
+ * Assist with testing of specific protected methods
+ */
+class i18nTextCollectorTest_Collector extends i18nTextCollector implements TestOnly {
+	public function getModules_Test($directory) {
+		return $this->getModules($directory);
+	}
+
+	public function resolveDuplicateConflicts_Test($entitiesByModule) {
+		return $this->resolveDuplicateConflicts($entitiesByModule);
+	}
+
+	public function getFileListForModule_Test($module) {
+		return parent::getFileListForModule($module);
+	}
+
+	public function getConflicts_Test($entitiesByModule) {
+		return parent::getConflicts($entitiesByModule);
+	}
 
 }

tests/parsers/ShortcodeParserTest.php

@@ -4,17 +4,17 @@
  * @subpackage tests
  */
 class ShortcodeParserTest extends SapphireTest {
-
+	
 	protected $arguments, $contents, $tagName, $parser;
 	protected $extra = array();
-
+	
 	public function setUp() {
 		ShortcodeParser::get('test')->register('test_shortcode', array($this, 'shortcodeSaver'));
 		$this->parser = ShortcodeParser::get('test');
-
+		
 		parent::setUp();
 	}
-
+	
 	/**
 	 * Tests that valid short codes that have not been registered are not replaced.
 	 */
@@ -37,7 +37,7 @@ class ShortcodeParserTest extends SapphireTest {
 			'<strong class="warning">[not_shortcode]</strong>',
 			$this->parser->parse('[not_shortcode]')
 		);
-
+		
 		ShortcodeParser::$error_behavior = ShortcodeParser::LEAVE;
 
 		$this->assertEquals('[not_shortcode]',
@@ -56,66 +56,66 @@ class ShortcodeParserTest extends SapphireTest {
 			$this->parser->parse('<img class="[not_shortcode]">')
 		);
 	}
-
+	
 	public function testSimpleTag() {
 		$tests = array(
-			'[test_shortcode]',
+			'[test_shortcode]', 
 			'[test_shortcode ]', '[test_shortcode,]', '[test_shortcode, ]'.
 			'[test_shortcode/]', '[test_shortcode /]', '[test_shortcode,/]', '[test_shortcode, /]'
 		);
-
+		
 		foreach($tests as $test) {
 			$this->parser->parse($test);
-
+			
 			$this->assertEquals(array(), $this->arguments, $test);
 			$this->assertEquals('', $this->contents, $test);
 			$this->assertEquals('test_shortcode', $this->tagName, $test);
 		}
 	}
-
+	
 	public function testOneArgument() {
 		$tests = array (
 			'[test_shortcode foo="bar"]', '[test_shortcode,foo="bar"]',
 			"[test_shortcode foo='bar']", "[test_shortcode,foo='bar']",
 			'[test_shortcode  foo  =  "bar"  /]', '[test_shortcode,  foo  =  "bar"  /]'
 		);
-
+		
 		foreach($tests as $test) {
 			$this->parser->parse($test);
-
+			
 			$this->assertEquals(array('foo' => 'bar'), $this->arguments, $test);
 			$this->assertEquals('', $this->contents, $test);
 			$this->assertEquals('test_shortcode', $this->tagName, $test);
 		}
 	}
-
+	
 	public function testMultipleArguments() {
 		$this->parser->parse('[test_shortcode foo = "bar",bar=\'foo\', baz="buz"]');
-
+		
 		$this->assertEquals(array('foo' => 'bar', 'bar' => 'foo', 'baz' => 'buz'), $this->arguments);
 		$this->assertEquals('', $this->contents);
 		$this->assertEquals('test_shortcode', $this->tagName);
 	}
-
+	
 	public function testEnclosing() {
 		$this->parser->parse('[test_shortcode]foo[/test_shortcode]');
-
+		
 		$this->assertEquals(array(), $this->arguments);
 		$this->assertEquals('foo', $this->contents);
 		$this->assertEquals('test_shortcode', $this->tagName);
 	}
-
+	
 	public function testEnclosingWithArguments() {
 		$this->parser->parse('[test_shortcode,foo = "bar",bar=\'foo\',baz="buz"]foo[/test_shortcode]');
-
+		
 		$this->assertEquals(array('foo' => 'bar', 'bar' => 'foo', 'baz' => 'buz'), $this->arguments);
 		$this->assertEquals('foo', $this->contents);
 		$this->assertEquals('test_shortcode', $this->tagName);
 	}
-
+	
 	public function testShortcodeEscaping() {
 		$this->assertEquals(
-			'[test_shortcode]',
+			'[test_shortcode]', 
 			$this->parser->parse('[[test_shortcode]]')
 		);
 
@@ -123,12 +123,12 @@ class ShortcodeParserTest extends SapphireTest {
 			'[test_shortcode /]',
 			$this->parser->parse('[[test_shortcode /]]')
 		);
-
+		
 		$this->assertEquals(
 			'[test_shortcode]content[/test_shortcode]',
 			$this->parser->parse('[[test_shortcode]content[/test_shortcode]]'
 		));
-
+		
 		$this->assertEquals(
 			'[test_shortcode]content',
 			$this->parser->parse('[[test_shortcode]][test_shortcode]content[/test_shortcode]')
@@ -138,12 +138,12 @@ class ShortcodeParserTest extends SapphireTest {
 			'[test_shortcode]content[/test_shortcode]content2',
 			$this->parser->parse('[[test_shortcode]content[/test_shortcode]][test_shortcode]content2[/test_shortcode]'
 		));
-
+		
 		$this->assertEquals(
 			'[[Doesnt strip double [ character if not a shortcode',
 			$this->parser->parse('[[Doesnt strip double [ character if not a [test_shortcode]shortcode[/test_shortcode]'
 		));
-
+		
 		$this->assertEquals(
 			'[[Doesnt shortcode get confused by double ]] characters',
 			$this->parser->parse(
@@ -155,7 +155,7 @@ class ShortcodeParserTest extends SapphireTest {
 		$this->assertEquals('', $this->parser->parse('[test_shortcode,foo=bar!,baz = buz123]'));
 		$this->assertEquals(array('foo' => 'bar!', 'baz' => 'buz123'), $this->arguments);
 	}
-
+	
 	public function testSpacesForDelimiter() {
 		$this->assertEquals('', $this->parser->parse('[test_shortcode foo=bar! baz = buz123]'));
 		$this->assertEquals(array('foo' => 'bar!', 'baz' => 'buz123'), $this->arguments);
@@ -167,22 +167,22 @@ class ShortcodeParserTest extends SapphireTest {
 			$this->parser->parse('[test_shortcode,id="1"/]more[test_shortcode,id="2"]content[/test_shortcode]'),
 			'Assert that self-closing tags are respected during parsing.'
 		);
-
+		
 		$this->assertEquals(2, $this->arguments['id']);
 	}
 
 	public function testConsecutiveTags() {
 		$this->assertEquals('', $this->parser->parse('[test_shortcode][test_shortcode]'));
 	}
-
+	
 	protected function assertEqualsIgnoringWhitespace($a, $b, $message = null) {
 		$this->assertEquals(preg_replace('/\s+/', '', $a), preg_replace('/\s+/', '', $b), $message);
 	}
-
+	
 	public function testtExtract() {
 		// Left extracts to before the current block
 		$this->assertEqualsIgnoringWhitespace(
-			'Code<div>FooBar</div>',
+			'Code<div>FooBar</div>', 
 			$this->parser->parse('<div>Foo[test_shortcode class=left]Code[/test_shortcode]Bar</div>')
 		);
 
@@ -194,16 +194,16 @@ class ShortcodeParserTest extends SapphireTest {
 
 		// Center splits the current block
 		$this->assertEqualsIgnoringWhitespace(
-			'<div>Foo</div>Code<div>Bar</div>',
+			'<div>Foo</div>Code<div>Bar</div>', 
 			$this->parser->parse('<div>Foo[test_shortcode class=center]Code[/test_shortcode]Bar</div>')
 		);
 
 		// Even if the immediate parent isn't a the current block
 		$this->assertEqualsIgnoringWhitespace(
-			'<div>Foo<b>Bar</b></div>Code<div><b>Baz</b>Qux</div>',
+			'<div>Foo<b>Bar</b></div>Code<div><b>Baz</b>Qux</div>', 
 			$this->parser->parse('<div>Foo<b>Bar[test_shortcode class=center]Code[/test_shortcode]Baz</b>Qux</div>')
 		);
-
+		
 		// No class means don't extract
 		$this->assertEqualsIgnoringWhitespace(
 			'<div>FooCodeBar</div>',
@@ -211,6 +211,37 @@ class ShortcodeParserTest extends SapphireTest {
 		);
 	}
 
+	public function testShortcodesInsideScriptTag() {
+		$this->assertEqualsIgnoringWhitespace(
+			'<script>hello</script>',
+			$this->parser->parse('<script>[test_shortcode]hello[/test_shortcode]</script>')
+		);
+	}
+
+	public function testNumericShortcodes() {
+		$this->assertEqualsIgnoringWhitespace(
+			'[2]',
+			$this->parser->parse('[2]')
+		);
+		$this->assertEqualsIgnoringWhitespace(
+			'<script>[2]</script>',
+			$this->parser->parse('<script>[2]</script>')
+		);
+
+		$this->parser->register('2', function($attributes, $content, $this, $tag, $extra) {
+			return 'this is 2';
+		});
+
+		$this->assertEqualsIgnoringWhitespace(
+			'this is 2',
+			$this->parser->parse('[2]')
+		);
+		$this->assertEqualsIgnoringWhitespace(
+			'<script>this is 2</script>',
+			$this->parser->parse('<script>[2]</script>')
+		);
+	}
+
 	public function testExtraContext() {
 		$this->parser->parse('<a href="[test_shortcode]">Test</a>');
 
@@ -220,7 +251,7 @@ class ShortcodeParserTest extends SapphireTest {
 	}
 
 	// -----------------------------------------------------------------------------------------------------------------
-
+	
 	/**
 	 * Stores the result of a shortcode parse in object properties for easy testing access.
 	 */
@@ -232,5 +263,5 @@ class ShortcodeParserTest extends SapphireTest {
 
 		return $content;
 	}
-
+	
 }