CMSPageHistoryController - Code Metrics - Inspection of "Merge pull request #1534 from open-sausages/pulls/..." - silverstripe/silverstripe-cms - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Push — master ( bf5119...d208a6 )

by Ingo

created 2016-07-01 03:09 UTC

CMSPageHistoryController B

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	419
Duplicated Lines	11.22 %

Coupling/Cohesion

Components	1
Dependencies	15

Importance

Changes	2
Bugs	0	Features	0

Metric	Value
wmc	47
c	2
b	0
f	0
lcom	1
cbo	15
dl	47
loc	419
rs	7.4481

11 Methods

Rating	Name	Duplication	Size	Complexity
A	getResponseNegotiator()	0	13	2
A	show()	14	14	2
A	compare()	17	17	2
A	getSilverStripeNavigator()	0	9	2
C	getEditForm()	0	76	9
C	VersionsForm()	0	82	9
B	doCompare()	8	27	3
B	doShowVersion()	8	24	5
A	ShowVersionForm()	0	8	2
C	CompareVersionsForm()	0	55	10
A	Breadcrumbs()	0	5	1

How to fix Duplicated Code Complexity

Duplicated Code

Duplicate code is one of the most pungent code smells. A rule that is often used is to re-structure code once it is duplicated in three or more places.

Common duplication problems, and corresponding solutions are:

If you have the same expression in different places: Extract expression to a method
If you have the same method in different sub-classes: Extract method, and pull up field to the parent class
If you have the same code in unrelated classes: Consider extracting the code to a new class, and injecting that class

Complex Class

Tip: Before tackling complexity, make sure that you eliminate any duplication first. This often can reduce the size of classes significantly.

Complex classes like CMSPageHistoryController often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.

Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.

While breaking up the class, it is a good idea to analyze how other classes use CMSPageHistoryController, and based on these observations, apply Extract Interface, too.

<?php

use SilverStripe\ORM\DataObject;
.
|-- OtherDir
|   |-- Bar.php
|   `-- Foo.php
`-- SomeDir
    `-- Foo.php
use SilverStripe\ORM\Versioning\Versioned;
.
|-- OtherDir
|   |-- Bar.php
|   `-- Foo.php
`-- SomeDir
    `-- Foo.php

/**
 * @package cms
 * @subpackage controllers
 */
class CMSPageHistoryController extends CMSMain {

	private static $url_segment = 'pages/history';

	private static $url_rule = '/$Action/$ID/$VersionID/$OtherVersionID';

	private static $url_priority = 42;

	private static $menu_title = 'History';

	private static $required_permission_codes = 'CMS_ACCESS_CMSMain';

	private static $session_namespace = 'CMSMain';


	private static $allowed_actions = array(

		'VersionsForm',
		'CompareVersionsForm',
		'show',
		'compare'
	);

	private static $url_handlers = array(

		'$Action/$ID/$VersionID/$OtherVersionID' => 'handleAction'
	);

	public function getResponseNegotiator() {
		$negotiator = parent::getResponseNegotiator();
		$controller = $this;
		$negotiator->setCallback('CurrentForm', function() use(&$controller) {
			$form = $controller->ShowVersionForm($controller->getRequest()->param('VersionID'));
			if($form) return $form->forTemplate();
			else return $controller->renderWith($controller->getTemplatesWithSuffix('_Content'));
		});
		$negotiator->setCallback('default', function() use(&$controller) {
			return $controller->renderWith($controller->getViewer('show'));
		});
		return $negotiator;
	}

	/**
	 * @param SS_HTTPRequest $request
	 * @return array
	 */
	public function show($request) {

		$form = $this->ShowVersionForm($request->param('VersionID'));

		$negotiator = $this->getResponseNegotiator();
		$controller = $this;
		$negotiator->setCallback('CurrentForm', function() use(&$controller, &$form) {
			return $form ? $form->forTemplate() : $controller->renderWith($controller->getTemplatesWithSuffix('_Content'));
		});
		$negotiator->setCallback('default', function() use(&$controller, &$form) {
			return $controller->customise(array('EditForm' => $form))->renderWith($controller->getViewer('show'));
		});

		return $negotiator->respond($request);
	}

	/**
	 * @param SS_HTTPRequest $request
	 * @return array
	 */
	public function compare($request) {

		$form = $this->CompareVersionsForm(
			$request->param('VersionID'),
			$request->param('OtherVersionID')
		);

		$negotiator = $this->getResponseNegotiator();
		$controller = $this;
		$negotiator->setCallback('CurrentForm', function() use(&$controller, &$form) {
			return $form ? $form->forTemplate() : $controller->renderWith($controller->getTemplatesWithSuffix('_Content'));
		});
		$negotiator->setCallback('default', function() use(&$controller, &$form) {
			return $controller->customise(array('EditForm' => $form))->renderWith($controller->getViewer('show'));
		});

		return $negotiator->respond($request);
	}

	public function getSilverStripeNavigator() {
		$record = $this->getRecord($this->currentPageID(), $this->getRequest()->param('VersionID'));
		if($record) {
			$navigator = new SilverStripeNavigator($record);
			return $navigator->renderWith($this->getTemplatesWithSuffix('_SilverStripeNavigator'));
class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}
		} else {
			return false;
class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}
		}
	}

	/**
	 * Returns the read only version of the edit form. Detaches all {@link FormAction}
	 * instances attached since only action relates to revert.
	 *
	 * Permission checking is done at the {@link CMSMain::getEditForm()} level.
	 *
	 * @param int $id ID of the record to show
	 * @param array $fields optional
	 * @param int $versionID
	 * @param int $compareID Compare mode
	 *
	 * @return Form
	 */
	public function getEditForm($id = null, $fields = null, $versionID = null, $compareID = null) {
		if(!$id) $id = $this->currentPageID();
0   == false // true
0   == null  // true
123 == false // false
123 == null  // false

// It is often better to use strict comparison
0 === false // false
0 === null  // false

		$record = $this->getRecord($id, $versionID);
		$versionID = ($record) ? $record->Version : $versionID;

		$form = parent::getEditForm($record, ($record) ? $record->getCMSFields() : null);
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
		// Respect permission failures from parent implementation
		if(!($form instanceof Form)) return $form;

		// TODO: move to the SilverStripeNavigator structure so the new preview can pick it up.
		//$nav = new SilverStripeNavigatorItem_ArchiveLink($record);

		$form->setActions(new FieldList(
			$revert = FormAction::create('doRollback', _t('CMSPageHistoryController.REVERTTOTHISVERSION', 'Revert to this version'))->setUseButtonTag(true)
		));

		$fields = $form->Fields();
		$fields->removeByName("Status");
		$fields->push(new HiddenField("ID"));
		$fields->push(new HiddenField("Version"));

		$fields = $fields->makeReadonly();

		if($compareID) {
0   == false // true
0   == null  // true
123 == false // false
123 == null  // false

// It is often better to use strict comparison
0 === false // false
0 === null  // false
			$link = Controller::join_links(
				$this->Link('show'),
				$id
			);

			$view = _t('CMSPageHistoryController.VIEW',"view");

			$message = _t(
				'CMSPageHistoryController.COMPARINGVERSION',
				"Comparing versions {version1} and {version2}.",
				array(
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
					'version1' => sprintf('%s (<a href="%s">%s</a>)', $versionID, Controller::join_links($link, $versionID), $view),
					'version2' => sprintf('%s (<a href="%s">%s</a>)', $compareID, Controller::join_links($link, $compareID), $view)
				)
			);

			$revert->setReadonly(true);
		} else {
			if($record->isLatestVersion()) {
				$message = _t('CMSPageHistoryController.VIEWINGLATEST', 'Currently viewing the latest version.');
			} else {
				$message = _t(
					'CMSPageHistoryController.VIEWINGVERSION',
					"Currently viewing version {version}.",
					array('version' => $versionID)
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
				);
			}
		}

		$fields->addFieldToTab('Root.Main',
			new LiteralField('CurrentlyViewingMessage', $this->customise(array(
				'Content' => $message,
				'Classes' => 'notice'
			))->renderWith(array('CMSMain_notice'))),
			"Title"
		);

		$form->setFields($fields->makeReadonly());
		$form->loadDataFrom(array(
			"ID" => $id,
			"Version" => $versionID,
		));

		if(($record && $record->isLatestVersion())) {
			$revert->setReadonly(true);
		}

		$form->removeExtraClass('cms-content');

		return $form;
	}


	/**
	 * Version select form. Main interface between selecting versions to view
	 * and comparing multiple versions.
	 *
	 * Because we can reload the page directly to a compare view (history/compare/1/2/3)
	 * this form has to adapt to those parameters as well.
	 *
	 * @return Form
	 */
	public function VersionsForm() {
		$id = $this->currentPageID();
		$page = $this->getRecord($id);
		$versionsHtml = '';

		$action = $this->getRequest()->param('Action');
		$versionID = $this->getRequest()->param('VersionID');
		$otherVersionID = $this->getRequest()->param('OtherVersionID');

		$showUnpublishedChecked = 0;
		$compareModeChecked = ($action == "compare");

		if($page) {
			$versions = $page->allVersions();
			$versionID = (!$versionID) ? $page->Version : $versionID;

			if($versions) {
				foreach($versions as $k => $version) {
					$active = false;

					if($version->Version == $versionID || $version->Version == $otherVersionID) {
						$active = true;

						if(!$version->WasPublished) $showUnpublishedChecked = 1;
					}

					$version->Active = ($active);
				}
			}

			$vd = new ViewableData();

			$versionsHtml = $vd->customise(array(
				'Versions' => $versions
			))->renderWith('CMSPageHistoryController_versions');
		}

		$fields = new FieldList(
			new CheckboxField(
				'ShowUnpublished',
				_t('CMSPageHistoryController.SHOWUNPUBLISHED','Show unpublished versions'),
				$showUnpublishedChecked
			),
			new CheckboxField(
				'CompareMode',
				_t('CMSPageHistoryController.COMPAREMODE', 'Compare mode (select two)'),
				$compareModeChecked
			),
			new LiteralField('VersionsHtml', $versionsHtml),
/**
 * @return array|string
 */
function returnsDifferentValues($x) {
    if ($x) {
        return 'foo';
    }

    return array();
}

$x = returnsDifferentValues($y);
if (is_array($x)) {
    // $x is an array.
}
			$hiddenID = new HiddenField('ID', false, "")
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
		);

		$actions = new FieldList(
			new FormAction(
				'doCompare', _t('CMSPageHistoryController.COMPAREVERSIONS','Compare Versions')
			),
			new FormAction(
				'doShowVersion', _t('CMSPageHistoryController.SHOWVERSION','Show Version')
			)
		);

		// Use <button> to allow full jQuery UI styling
		foreach($actions->dataFields() as $action) $action->setUseButtonTag(true);

		$negotiator = $this->getResponseNegotiator();
$myVar = 'Value';
$higher = false;

if (rand(1, 6) > 3) {
    $higher = true;
} else {
    $higher = false;
}
		$form = Form::create(
			$this,
			'VersionsForm',
			$fields,
			$actions
		)->setHTMLID('Form_VersionsForm');
		$form->loadDataFrom($this->getRequest()->requestVars());

		$hiddenID->setValue($id);
		$form->unsetValidator();

		$form
			->addExtraClass('cms-versions-form') // placeholder, necessary for $.metadata() to work
			->setAttribute('data-link-tmpl-compare', Controller::join_links($this->Link('compare'), '%s', '%s', '%s'))
			->setAttribute('data-link-tmpl-show', Controller::join_links($this->Link('show'), '%s', '%s'));

		return $form;
	}

	/**
	 * Process the {@link VersionsForm} compare function between two pages.
	 *
	 * @param array
	 * @param Form
	 *
	 * @return html
	 */
	public function doCompare($data, $form) {

		$versions = $data['Versions'];
		if(count($versions) < 2) return null;

		$id = $this->currentPageID();
$myVar = 'Value';
$higher = false;

if (rand(1, 6) > 3) {
    $higher = true;
} else {
    $higher = false;
}
		$version1 = array_shift($versions);
		$version2 = array_shift($versions);

		$form = $this->CompareVersionsForm($version1, $version2);

		// javascript solution, render into template
		if($this->getRequest()->isAjax()) {

			return $this->customise(array(
				"EditForm" => $form
			))->renderWith(array(
				$this->class . '_EditForm',
				'LeftAndMain_Content'
			));
		}

		// non javascript, redirect the user to the page
		$this->redirect(Controller::join_links(
			$this->Link('compare'),
			$version1,
			$version2
		));
	}

	/**
	 * Process the {@link VersionsForm} show version function. Only requires
	 * one page to be selected.
	 *
	 * @param array
	 * @param Form
	 *
	 * @return html
	 */
	public function doShowVersion($data, $form) {

		$versionID = null;

		if(isset($data['Versions']) && is_array($data['Versions'])) {
			$versionID  = array_shift($data['Versions']);
		}

		if(!$versionID) return;

		if($request->isAjax()) {

			return $this->customise(array(
				"EditForm" => $this->ShowVersionForm($versionID)
			))->renderWith(array(
				$this->class . '_EditForm',
				'LeftAndMain_Content'
			));
		}

		// non javascript, redirect the user to the page
		$this->redirect(Controller::join_links(
			$this->Link('version'),
			$versionID
		));
	}

	/**
	 * @param int|null $versionID
	 * @return Form
	 */
	public function ShowVersionForm($versionID = null) {
		if(!$versionID) return null;
0   == false // true
0   == null  // true
123 == false // false
123 == null  // false

// It is often better to use strict comparison
0 === false // false
0 === null  // false

		$id = $this->currentPageID();
		$form = $this->getEditForm($id, null, $versionID);

		return $form;
	}

	/**
	 * @param int $versionID
	 * @param int $otherVersionID
	 * @return Form
	 */
	public function CompareVersionsForm($versionID, $otherVersionID) {
		if($versionID > $otherVersionID) {
			$toVersion = $versionID;
			$fromVersion = $otherVersionID;
		} else {
			$toVersion = $otherVersionID;
			$fromVersion = $versionID;
		}

		if(!$toVersion || !$fromVersion) return false;
class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}

		$id = $this->currentPageID();
		$page = DataObject::get_by_id("SiteTree", $id);

		if($page && !$page->canView()) {
			return Security::permissionFailure($this);

		}

		$record = $page->compareVersions($fromVersion, $toVersion);

		$fromVersionRecord = Versioned::get_version('SiteTree', $id, $fromVersion);
		$toVersionRecord = Versioned::get_version('SiteTree', $id, $toVersion);

		if(!$fromVersionRecord) {
			user_error("Can't find version $fromVersion of page $id", E_USER_ERROR);
		}

		if(!$toVersionRecord) {
			user_error("Can't find version $toVersion of page $id", E_USER_ERROR);
		}

		if($record) {
			$form = $this->getEditForm($id, null, null, true);
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
			$form->setActions(new FieldList());
			$form->addExtraClass('compare');

			// Comparison views shouldn't be editable.
			// Its important to convert fields *before* loading data,
			// as the comparison output is HTML and not valid values for the various field types
			$readonlyFields = $form->Fields()->makeReadonly();
			$form->setFields($readonlyFields);

			$form->loadDataFrom($record);
			$form->loadDataFrom(array(
				"ID" => $id,
				"Version" => $fromVersion,
			));

			foreach($form->Fields()->dataFields() as $field) {
				$field->dontEscape = true;
			}

			return $form;
		}
	}

	public function Breadcrumbs($unlinked = false) {
		$crumbs = parent::Breadcrumbs($unlinked);
		$crumbs[0]->Title = _t('CMSPagesController.MENUTITLE');
		return $crumbs;
	}

}


1			<?php
2
3			use SilverStripe\ORM\DataObject;
			0 ignored issues – show Bug introduced 2016-06-23 23:49 UTC by Report Bug Copy Issue Report This use statement conflicts with another class in this namespace, `DataObject`. Let’s assume that you have a directory layout like this: . \|-- OtherDir \| \|-- Bar.php \| `-- Foo.php `-- SomeDir `-- Foo.php and let’s assume the following content of `Bar.php`: // Bar.php namespace OtherDir; use SomeDir\Foo; // This now conflicts the class OtherDir\Foo If both files `OtherDir/Foo.php` and `SomeDir/Foo.php` are loaded in the same runtime, you will see a PHP error such as the following: PHP Fatal error: Cannot use SomeDir\Foo as Foo because the name is already in use in OtherDir/Foo.php However, as `OtherDir/Foo.php` does not necessarily have to be loaded and the error is only triggered if it is loaded before `OtherDir/Bar.php`, this problem might go unnoticed for a while. In order to prevent this error from surfacing, you must import the namespace with a different alias: // Bar.php namespace OtherDir; use SomeDir\Foo as SomeDirFoo; // There is no conflict anymore. Loading history...
4			use SilverStripe\ORM\Versioning\Versioned;
			0 ignored issues – show Bug introduced 2016-06-23 23:49 UTC by Report Bug Copy Issue Report This use statement conflicts with another class in this namespace, `Versioned`. Let’s assume that you have a directory layout like this: . \|-- OtherDir \| \|-- Bar.php \| `-- Foo.php `-- SomeDir `-- Foo.php and let’s assume the following content of `Bar.php`: // Bar.php namespace OtherDir; use SomeDir\Foo; // This now conflicts the class OtherDir\Foo If both files `OtherDir/Foo.php` and `SomeDir/Foo.php` are loaded in the same runtime, you will see a PHP error such as the following: PHP Fatal error: Cannot use SomeDir\Foo as Foo because the name is already in use in OtherDir/Foo.php However, as `OtherDir/Foo.php` does not necessarily have to be loaded and the error is only triggered if it is loaded before `OtherDir/Bar.php`, this problem might go unnoticed for a while. In order to prevent this error from surfacing, you must import the namespace with a different alias: // Bar.php namespace OtherDir; use SomeDir\Foo as SomeDirFoo; // There is no conflict anymore. Loading history...
5
6			/**
7			* @package cms
8			* @subpackage controllers
9			*/
10			class CMSPageHistoryController extends CMSMain {
11
12			private static $url_segment = 'pages/history';
			0 ignored issues – show Comprehensibility introduced 2015-11-26 02:50 UTC by Report Bug Copy Issue Report Consider using a different property name as you override a private property of the parent class. Loading history...
13			private static $url_rule = '/$Action/$ID/$VersionID/$OtherVersionID';
			0 ignored issues – show Comprehensibility introduced 2015-11-26 02:50 UTC by Report Bug Copy Issue Report Consider using a different property name as you override a private property of the parent class. Loading history...
14			private static $url_priority = 42;
			0 ignored issues – show Comprehensibility introduced 2015-11-26 02:50 UTC by Report Bug Copy Issue Report Consider using a different property name as you override a private property of the parent class. Loading history...
15			private static $menu_title = 'History';
			0 ignored issues – show Comprehensibility introduced 2015-11-26 02:50 UTC by Report Bug Copy Issue Report Consider using a different property name as you override a private property of the parent class. Loading history...
16			private static $required_permission_codes = 'CMS_ACCESS_CMSMain';
			0 ignored issues – show Comprehensibility introduced 2015-11-26 02:50 UTC by Report Bug Copy Issue Report Consider using a different property name as you override a private property of the parent class. Loading history...
17			private static $session_namespace = 'CMSMain';
			0 ignored issues – show Comprehensibility introduced 2015-11-26 02:50 UTC by Report Bug Copy Issue Report Consider using a different property name as you override a private property of the parent class. Loading history...
18
19			private static $allowed_actions = array(
			0 ignored issues – show Comprehensibility introduced 2015-11-26 02:50 UTC by Report Bug Copy Issue Report Consider using a different property name as you override a private property of the parent class. Loading history...
20			'VersionsForm',
21			'CompareVersionsForm',
22			'show',
23			'compare'
24			);
25
26			private static $url_handlers = array(
			0 ignored issues – show Comprehensibility introduced 2015-11-26 02:50 UTC by Report Bug Copy Issue Report Consider using a different property name as you override a private property of the parent class. Loading history...
27			'$Action/$ID/$VersionID/$OtherVersionID' => 'handleAction'
28			);
29
30			public function getResponseNegotiator() {

silverstripe / silverstripe-cms