Merge pull request #2347 from simonwelsh/3.1

Scrutinizer fixes

.scrutinizer.yml

@@ -3,3 +3,5 @@ tools:
         -
             scope: file
             command: php tests/phpcs_runner.php %pathname%
+            filter:
+               excluded_paths: ["*/css/*", "css/*", "thirdparty/*", "*/jquery-changetracker/*", "parsers/HTML/BBCodeParser/*", "*/SSTemplateParser.php$", "docs/*", "*/images/*"]

admin/tests/LeftAndMainTest.php

@@ -49,8 +49,10 @@ class LeftAndMainTest extends FunctionalTest {
 		$this->session()->inst_set('loggedInAs', $admin->ID);
 		$response = $this->get('LeftAndMainTest_Controller');
 
-		$this->assertRegExp('/tests\/assets\/LeftAndMainTest.css/i', $response->getBody(), "body should contain custom css");
-		$this->assertRegExp('/tests\/assets\/LeftAndMainTest.js/i', $response->getBody(), "body should contain custom js");
+		$this->assertRegExp('/tests\/assets\/LeftAndMainTest.css/i', $response->getBody(),
+			"body should contain custom css");
+		$this->assertRegExp('/tests\/assets\/LeftAndMainTest.js/i', $response->getBody(),
+			"body should contain custom js");
 	}
 
 	/**

api/XMLDataFormatter.php

@@ -96,7 +96,6 @@ class XMLDataFormatter extends DataFormatter {
 				$items = $obj->$relName();
 				if ($items) {
 					foreach($items as $item) {
-						//$href = Director::absoluteURL($this->config()->api_base . "$className/$id/$relName/$item->ID");
 						$href = Director::absoluteURL($this->config()->api_base . "$relClass/$item->ID");
 						$xml .= "<$relClass href=\"$href.xml\" id=\"{$item->ID}\"></$relClass>\n";
 					}

cli-script.php

@@ -75,7 +75,8 @@ require_once("model/DB.php");
 if(!isset($databaseConfig) || !isset($databaseConfig['database']) || !$databaseConfig['database']) {
 	echo "\nPlease configure your database connection details.  You can do this by creating a file
 called _ss_environment.php in either of the following locations:\n\n";
-	echo " - " .  BASE_PATH  . DIRECTORY_SEPARATOR . "_ss_environment.php\n - " . dirname(BASE_PATH) . DIRECTORY_SEPARATOR . "_ss_environment.php\n\n";
+	echo " - " .  BASE_PATH  . DIRECTORY_SEPARATOR . "_ss_environment.php\n - ";
+	echo dirname(BASE_PATH) . DIRECTORY_SEPARATOR . "_ss_environment.php\n\n";
 	echo <<<ENVCONTENT
 
 Put the following content into this file:

dev/CsvBulkLoader.php

@@ -210,7 +210,8 @@ class CsvBulkLoader extends BulkLoader {
 			if(is_string($duplicateCheck)) {
 				$SQL_fieldName = Convert::raw2sql($duplicateCheck); 
 				
-				if(!isset($record[$SQL_fieldName]) || empty($record[$SQL_fieldName])) { //skip current duplicate check if field value is empty
+				if(!isset($record[$SQL_fieldName]) || empty($record[$SQL_fieldName])) {
+					//skip current duplicate check if field value is empty
 					continue;
 				}
 

dev/SapphireTest.php

@@ -501,12 +501,24 @@ class SapphireTest extends PHPUnit_Framework_TestCase {
 		}
 	}
 
-	public static function assertContains($needle, $haystack, $message = '', $ignoreCase = FALSE, $checkForObjectIdentity = TRUE) {
+	public static function assertContains(
+		$needle,
+		$haystack,
+		$message = '',
+		$ignoreCase = FALSE,
+		$checkForObjectIdentity = TRUE
+	) {
 		if ($haystack instanceof DBField) $haystack = (string)$haystack;
 		parent::assertContains($needle, $haystack, $message, $ignoreCase, $checkForObjectIdentity);
 	}
 
-	public static function assertNotContains($needle, $haystack, $message = '', $ignoreCase = FALSE, $checkForObjectIdentity = TRUE) {
+	public static function assertNotContains(
+		$needle,
+		$haystack,
+		$message = '',
+		$ignoreCase = FALSE,
+		$checkForObjectIdentity = TRUE
+	) {
 		if ($haystack instanceof DBField) $haystack = (string)$haystack;
 		parent::assertNotContains($needle, $haystack, $message, $ignoreCase, $checkForObjectIdentity);
 	}

filesystem/ImagickBackend.php

@@ -255,9 +255,11 @@ class ImagickBackend extends Imagick implements Image_Backend {
 		$new->setBackgroundColor($backgroundColor);
 		
 		if(($geo['width']/$width) < ($geo['height']/$height)){
-			$new->cropImage($geo['width'], floor($height*$geo['width']/$width), 0, (($geo['height']-($height*$geo['width']/$width))/2));
+			$new->cropImage($geo['width'], floor($height*$geo['width']/$width),
+				0, (($geo['height']-($height*$geo['width']/$width))/2));
 		}else{
-		    $new->cropImage(ceil($width*$geo['height']/$height), $geo['height'], (($geo['width']-($width*$geo['height']/$height))/2), 0);
+			$new->cropImage(ceil($width*$geo['height']/$height), $geo['height'],
+				(($geo['width']-($width*$geo['height']/$height))/2), 0);
 		}
 		$new->ThumbnailImage($width,$height,true);
 		return $new;

forms/CheckboxField.php

@@ -56,7 +56,9 @@ class CheckboxField_Readonly extends ReadonlyField {
 	}
 
 	public function Value() {
-		return Convert::raw2xml($this->value ? _t('CheckboxField.YESANSWER', 'Yes') : _t('CheckboxField.NOANSWER', 'No'));
+		return Convert::raw2xml($this->value ?
+			_t('CheckboxField.YESANSWER', 'Yes') :
+			_t('CheckboxField.NOANSWER', 'No'));
 	}
 
 }

forms/CheckboxSetField.php

@@ -181,7 +181,8 @@ class CheckboxSetField extends OptionsetField {
 	public function saveInto(DataObjectInterface $record) {
 		$fieldname = $this->name;
 		$relation = ($fieldname && $record && $record->hasMethod($fieldname)) ? $record->$fieldname() : null;
-		if($fieldname && $record && $relation && ($relation instanceof RelationList || $relation instanceof UnsavedRelationList)) {
+		if($fieldname && $record && $relation &&
+			($relation instanceof RelationList || $relation instanceof UnsavedRelationList)) {
 			$idList = array();
 			if($this->value) foreach($this->value as $id => $bool) {
 				if($bool) {

forms/HtmlEditorField.php

@@ -360,7 +360,8 @@ class HtmlEditorField_Toolbar extends RequestHandler {
 				'<h4>' . sprintf($numericLabelTmpl, '1', _t('HtmlEditorField.ADDURL', 'Add URL')) . '</h4>'),
 			$remoteURL = new TextField('RemoteURL', 'http://'),
 			new LiteralField('addURLImage',
-				'<button class="action ui-action-constructive ui-button field add-url" data-icon="addMedia">'._t('HtmlEditorField.BUTTONADDURL', 'Add url').'</button>')
+				'<button class="action ui-action-constructive ui-button field add-url" data-icon="addMedia">' .
+				_t('HtmlEditorField.BUTTONADDURL', 'Add url').'</button>')
 		);
 
 		$remoteURL->addExtraClass('remoteurl');
@@ -640,7 +641,8 @@ class HtmlEditorField_Toolbar extends RequestHandler {
 	 */
 	protected function getFieldsForImage($url, $file) {
 		if($file->File instanceof Image) {
-			$formattedImage = $file->File->generateFormattedImage('SetWidth', Config::inst()->get('Image', 'asset_preview_width'));
+			$formattedImage = $file->File->generateFormattedImage('SetWidth',
+				Config::inst()->get('Image', 'asset_preview_width'));
 			$thumbnailURL = $formattedImage ? $formattedImage->URL : $url;	
 		} else {
 			$thumbnailURL = $url;

forms/ListboxField.php

@@ -179,7 +179,8 @@ class ListboxField extends DropdownField {
 		if($this->multiple) {
 			$fieldname = $this->name;
 			$relation = ($fieldname && $record && $record->hasMethod($fieldname)) ? $record->$fieldname() : null;
-			if($fieldname && $record && $relation && ($relation instanceof RelationList || $relation instanceof UnsavedRelationList)) {
+			if($fieldname && $record && $relation &&
+				($relation instanceof RelationList || $relation instanceof UnsavedRelationList)) {
 				$idList = (is_array($this->value)) ? array_values($this->value) : array();
 				if(!$record->ID) {
 					$record->write(); // record needs to have an ID in order to set relationships

i18n/i18n.php

@@ -676,7 +676,8 @@ class i18n extends Object implements TemplateGlobalProvider {
 		),
 		'be' => array(
 			'name' => 'Belarusian',
-			'native' => 'Беларуская мова'
+			'native' =>
+				'Беларуская мова'
 		),
 		'bn' => array(
 			'name' => 'Bengali', 
@@ -1024,7 +1025,8 @@ class i18n extends Object implements TemplateGlobalProvider {
 		),
 		'be_BY' => array(
 			'name' => 'Belarusian',
-			'native' => 'Беларуская мова'
+			'native' =>
+				'Беларуская мова'
 		),
 		'bn_BD' => array(
 			'name' => 'Bengali',

model/Hierarchy.php

@@ -98,13 +98,15 @@ class Hierarchy extends DataExtension {
 	 *                          should not change this.
 	 * @param int $nodeCountThreshold See {@link self::$node_threshold_total}
 	 * @param callable $nodeCountCallback Called with the node count, which gives the callback an opportunity
-	 *                 to intercept the query. Useful e.g. to avoid excessive children listings (Arguments: $parent, $numChildren)
+	 *                 to intercept the query. Useful e.g. to avoid excessive children listings
+	 *                 (Arguments: $parent, $numChildren)
 	 *                          
 	 * @return string
 	 */
 	public function getChildrenAsUL($attributes = "", $titleEval = '"<li>" . $child->Title', $extraArg = null, 
 			$limitToMarked = false, $childrenMethod = "AllChildrenIncludingDeleted", 
-			$numChildrenMethod = "numChildren", $rootCall = true, $nodeCountThreshold = null, $nodeCountCallback = null) {
+			$numChildrenMethod = "numChildren", $rootCall = true,
+			$nodeCountThreshold = null, $nodeCountCallback = null) {
 
 		if(!is_numeric($nodeCountThreshold)) {
 			$nodeCountThreshold = Config::inst()->get('Hierarchy', 'node_threshold_total');
@@ -154,8 +156,8 @@ class Hierarchy extends DataExtension {
 							$output .= $nodeCountWarning;
 							$child->markClosed();
 						} else {
-							$output .= $child->getChildrenAsUL("", $titleEval, $extraArg, $limitToMarked, $childrenMethod,
-								$numChildrenMethod, false, $nodeCountThreshold);	
+							$output .= $child->getChildrenAsUL("", $titleEval, $extraArg, $limitToMarked,
+								$childrenMethod,	$numChildrenMethod, false, $nodeCountThreshold);
 						}
 					} elseif($child->isTreeOpened()) {
 						// Since we're not loading children, don't mark it as open either

parsers/ShortcodeParser.php

@@ -247,10 +247,12 @@ class ShortcodeParser {
 					$err = 'Close tag "'.$tags[$i]['close'].'" is the first found tag, so has no related open tag';
 				}
 				else if (!$tags[$i-1]['open']) {
-					$err = 'Close tag "'.$tags[$i]['close'].'" preceded by another close tag "'.$tags[$i-1]['close'].'"';
+					$err = 'Close tag "'.$tags[$i]['close'].'" preceded by another close tag "'.
+							$tags[$i-1]['close'].'"';
 				}
 				else if ($tags[$i]['close'] != $tags[$i-1]['open']) {
-					$err = 'Close tag "'.$tags[$i]['close'].'" doesn\'t match preceding open tag "'.$tags[$i-1]['open'].'"';
+					$err = 'Close tag "'.$tags[$i]['close'].'" doesn\'t match preceding open tag "'.
+							$tags[$i-1]['open'].'"';
 				}
 		
 				if($err) {
@@ -332,7 +334,8 @@ class ShortcodeParser {
 			$tags = $this->extractTags($node->nodeValue);
 
 			if($tags) {
-				$node->nodeValue = $this->replaceTagsWithText($node->nodeValue, $tags, function($idx, $tag) use ($parser){
+				$node->nodeValue = $this->replaceTagsWithText($node->nodeValue, $tags,
+					function($idx, $tag) use ($parser){
 					$content = $parser->callShortcode($tag['open'], $tag['attrs'], $tag['content']);
 
 					if ($content === false) {
@@ -381,7 +384,8 @@ class ShortcodeParser {
 			do {
 				$parent = $parent->parentNode;
 		}
-			while($parent instanceof DOMElement && !in_array(strtolower($parent->tagName), self::$block_level_elements));
+			while($parent instanceof DOMElement &&
+				!in_array(strtolower($parent->tagName), self::$block_level_elements));
 
 			$node->setAttribute('data-parentid', count($parents));
 			$parents[] = $parent;
@@ -451,7 +455,8 @@ class ShortcodeParser {
 		else if($location == self::INLINE) {
 			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', 
+					'Requested to insert block tag '.$node->tagName.
+					' inline - probably this will break HTML compliance', 
 					E_USER_WARNING
 				);
 			}
@@ -549,7 +554,8 @@ class ShortcodeParser {
 
 			if(!$parent) {
 				if($location !== self::INLINE) {
-					user_error("Parent block for shortcode couldn't be found, but location wasn't INLINE", E_USER_ERROR);
+					user_error("Parent block for shortcode couldn't be found, but location wasn't INLINE",
+						E_USER_ERROR);
 				}
 			}
 			else {

security/Group.php

@@ -113,11 +113,13 @@ class Group extends DataObject {
 						} elseif($record && $record->ID) {
 							// TODO Mark disabled once chosen.js supports it
 							// $groupsField->setDisabledItems(array($group->ID));
-							$form->Fields()->replaceField('DirectGroups', $groupsField->performReadonlyTransformation());
+							$form->Fields()->replaceField('DirectGroups',
+								$groupsField->performReadonlyTransformation());
 						}
 					}
 				});
-			$memberList = GridField::create('Members',false, $this->DirectMembers(), $config)->addExtraClass('members_grid');
+			$memberList = GridField::create('Members',false, $this->DirectMembers(), $config)
+				->addExtraClass('members_grid');
 			// @todo Implement permission checking on GridField
 			//$memberList->setPermissions(array('edit', 'delete', 'export', 'add', 'inlineadd'));
 			$fields->addFieldToTab('Root.Members', $memberList);

tests/control/HTTPTest.php

@@ -130,7 +130,8 @@ class HTTPTest extends SapphireTest {
 			// background-image
 			// Note that using /./ in urls is absolutely acceptable
 			$test->assertEquals(
-				'<div style="background-image: url(\'http://www.silverstripe.org/./images/mybackground.gif\');">Content</div>',
+				'<div style="background-image: url(\'http://www.silverstripe.org/./images/mybackground.gif\');">'.
+				'Content</div>',
 				HTTP::absoluteURLs('<div style="background-image: url(\'./images/mybackground.gif\');">Content</div>')
 			);
 			
@@ -169,7 +170,8 @@ class HTTPTest extends SapphireTest {
 			// background
 			// Note that using /./ in urls is absolutely acceptable
 			$test->assertEquals(
-				'<div background="http://www.silverstripe.org/./themes/silverstripe/images/nav-bg-repeat-2.png">SS Blog</div>',
+				'<div background="http://www.silverstripe.org/./themes/silverstripe/images/nav-bg-repeat-2.png">'.
+				'SS Blog</div>',
 				HTTP::absoluteURLs('<div background="./themes/silverstripe/images/nav-bg-repeat-2.png">SS Blog</div>')
 			);
 			
@@ -210,8 +212,10 @@ class HTTPTest extends SapphireTest {
 
 			// data uri
 			$test->assertEquals(
-				'<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==" alt="Red dot" />',
-				HTTP::absoluteURLs('<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==" alt="Red dot" />'),
+				'<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38'.
+				'GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==" alt="Red dot" />',
+				HTTP::absoluteURLs('<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAH'.
+					'ElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==" alt="Red dot" />'),
 				'Data URI links are not rewritten'
 			);
 

tests/core/CoreTest.php

@@ -24,11 +24,13 @@ class CoreTest extends SapphireTest {
 			$user = getTempFolderUsername();
 
 			// A typical Windows location for where sites are stored on IIS
-			$this->assertEquals(sys_get_temp_dir() . '/silverstripe-cacheC--inetpub-wwwroot-silverstripe-test-project/' . $user,
+			$this->assertEquals(sys_get_temp_dir() .
+				'/silverstripe-cacheC--inetpub-wwwroot-silverstripe-test-project/' . $user,
 				getTempFolder('C:\\inetpub\\wwwroot\\silverstripe-test-project'));
 
 			// A typical Mac OS X location for where sites are stored
-			$this->assertEquals(sys_get_temp_dir() . '/silverstripe-cache-Users-joebloggs-Sites-silverstripe-test-project/' . $user,
+			$this->assertEquals(sys_get_temp_dir() .
+				'/silverstripe-cache-Users-joebloggs-Sites-silverstripe-test-project/' . $user,
 				getTempFolder('/Users/joebloggs/Sites/silverstripe-test-project'));
 
 			// A typical Linux location for where sites are stored

tests/core/manifest/ConfigStaticManifestTest.php

@@ -28,6 +28,7 @@ DOC;
 nowdoc
 DOC;
 
+	// @codingStandardsIgnoreStart
 	// Assigning multiple values
 	static $onone, $onull = null, $oint = 1, $ofloat = 2.5, $ostring = 'string', $oarray = array(1, 2, array(3, 4), 5), $oheredoc = <<<DOC
 heredoc
@@ -35,6 +36,7 @@ DOC
 , $onowdoc = <<<'DOC'
 nowdoc
 DOC;
+	// @codingStandardsIgnoreEnd
 
 	static
 		$mnone,

tests/forms/RequirementsTest.php

@@ -256,11 +256,13 @@ class RequirementsTest extends SapphireTest {
 		$html = $backend->includeInHTML(false, self::$html_template);
 
 		/* Javascript has correct path */
-		$this->assertTrue((bool)preg_match('/src=".*\/RequirementsTest_a\.js\?m=\d\d+&test=1&test=2&test=3/', $html),
+		$this->assertTrue(
+			(bool)preg_match('/src=".*\/RequirementsTest_a\.js\?m=\d\d+&test=1&test=2&test=3/',$html),
 			'javascript has correct path'); 
 
 		/* CSS has correct path */
-		$this->assertTrue((bool)preg_match('/href=".*\/RequirementsTest_a\.css\?m=\d\d+&test=1&test=2&test=3/',$html),
+		$this->assertTrue(
+			(bool)preg_match('/href=".*\/RequirementsTest_a\.css\?m=\d\d+&test=1&test=2&test=3/',$html),
 			'css has correct path');
 	}
 	

tests/model/HTMLTextTest.php

@@ -103,7 +103,8 @@ class HTMLTextTest extends SapphireTest {
 			'<h1>should ignore</h1><p>First Mr. sentence. Second sentence.</p>' => 'First Mr. sentence.',
 			"<h1>should ignore</h1><p>Sentence with {$many}words. Second sentence.</p>"
 				=> "Sentence with {$many}words.",
-			'<p>This classic picture book features a repetitive format that lends itself to audience interaction.&nbsp; Illustrator Eric Carle submitted new, bolder artwork for the 25th anniversary edition.</p>'
+			'<p>This classic picture book features a repetitive format that lends itself to audience interaction.'.
+			'&nbsp; Illustrator Eric Carle submitted new, bolder artwork for the 25th anniversary edition.</p>'
 				=> 'This classic picture book features a repetitive format that lends itself to audience interaction.'
 		);
 		

tests/model/ValidationExceptionTest.php

@@ -78,6 +78,7 @@ class ValidationExceptionTest extends SapphireTest
 		$this->assertEquals(E_USER_WARNING, $exception->getCode());
 		$this->assertEquals('An error has occurred', $exception->getMessage());
 		$this->assertEquals(false, $exception->getResult()->valid());
-		$this->assertEquals('A spork is not a knife; A knife is not a back scratcher', $exception->getResult()->message());
+		$this->assertEquals('A spork is not a knife; A knife is not a back scratcher',
+			$exception->getResult()->message());
 	}
 }

tests/parsers/ShortcodeParserTest.php

@@ -145,8 +145,9 @@ class ShortcodeParserTest extends SapphireTest {
 		
 		$this->assertEquals(
 			'[[Doesnt shortcode get confused by double ]] characters',
-			$this->parser->parse('[[Doesnt [test_shortcode]shortcode[/test_shortcode] get confused by double ]] characters'
-		));
+			$this->parser->parse(
+				'[[Doesnt [test_shortcode]shortcode[/test_shortcode] get confused by double ]] characters')
+		);
 	}
 
 	public function testUnquotedArguments() {

tests/phpcs_runner.php

@@ -12,6 +12,10 @@ if(!empty($_SERVER['argv'][1])) {
 
 $result = array('comments' => array());
 
+$extension = pathinfo($path, PATHINFO_EXTENSION);
+
+// Whitelist of extensions to check (default phpcs list)
+if(in_array($extension, array('php', 'js', 'inc', 'css'))) {
 	// Run each sniff
 
 	// phpcs --encoding=utf-8 --standard=framework/tests/phpcs/tabs.xml
@@ -19,7 +23,7 @@ run_sniff('tabs.xml', $path, $result);
 
 	// phpcs --encoding=utf-8 --tab-width=4 --standard=framework/tests/phpcs/ruleset.xml
 	run_sniff('ruleset.xml', $path, $result, '--tab-width=4');
-
+}
 echo json_encode($result);
 
 function run_sniff($standard, $path, array &$result, $extraFlags = '') {

view/SSTemplateParser.php.inc

@@ -1002,7 +1002,8 @@ class SSTemplateParser extends Parser {
 		// non-dynamically calculated
 		$text = preg_replace(
 			'/href\s*\=\s*\"\#/', 
-			'href="\' . (Config::inst()->get(\'SSViewer\', \'rewrite_hash_links\') ? strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . 
+			'href="\' . (Config::inst()->get(\'SSViewer\', \'rewrite_hash_links\') ?' .
+			' strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . 
 				\'#',
 			$text
 		);