CMSPageHistoryController - Code Metrics - Inspection of "Fix history comparison fields" - silverstripe/silverstripe-cms - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — master (#1749)

by Damian

created 2017-02-28 03:35 UTC

CMSPageHistoryController D

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	470
Duplicated Lines	10 %

Coupling/Cohesion

Components	1
Dependencies	20

Importance

Changes

Metric	Value
wmc	50
lcom	1
cbo	20
dl	47
loc	470
rs	4.05
c	0
b	0
f	0

12 Methods

Rating	Name	Duplication	Size	Complexity
C	VersionsForm()	0	89	9
B	doCompare()	8	29	3
B	doShowVersion()	8	28	5
A	ShowVersionForm()	0	11	2
A	getResponseNegotiator()	0	17	2
A	show()	14	15	2
A	compare()	17	18	2
A	getSilverStripeNavigator()	0	10	2
C	getEditForm()	0	80	9
C	CompareVersionsForm()	0	58	10
A	Breadcrumbs()	0	6	1
A	transformReadonly()	0	11	3

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

namespace SilverStripe\CMS\Controllers;

use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Control\Controller;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Forms\CheckboxField;
use SilverStripe\Forms\CompositeField;
use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\Form;
use SilverStripe\Forms\FormAction;
use SilverStripe\Forms\HiddenField;
use SilverStripe\Forms\HTMLReadonlyField;
use SilverStripe\Forms\LiteralField;
use SilverStripe\ORM\FieldType\DBField;
use SilverStripe\ORM\FieldType\DBHTMLText;
use SilverStripe\ORM\Versioning\Versioned;
use SilverStripe\Security\Security;
use SilverStripe\View\ArrayData;
use SilverStripe\View\ViewableData;

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 $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 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 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'));
        } 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) {
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();
        }

        $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(
                    '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()) {
class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
                $message = _t('CMSPageHistoryController.VIEWINGLATEST', 'Currently viewing the latest version.');
            } else {
                $message = _t(
                    'CMSPageHistoryController.VIEWINGVERSION',
                    "Currently viewing version {version}.",
                    array('version' => $versionID)
                );
            }
        }

        $fields->fieldByName('Root.Main')->unshift(
            new LiteralField('CurrentlyViewingMessage', ArrayData::create(array(
                'Content' => DBField::create_field('HTMLFragment', $message),
                'Classes' => 'notice'
            ))->renderWith($this->getTemplatesWithSuffix('_notice')))
        );

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

        if (($record && $record->isLatestVersion())) {
class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
            $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();
class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
            $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($this->getTemplatesWithSuffix('_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) {
            /** @var FormAction $action */
            $action->setUseButtonTag(true);
        }

        $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 $data
     * @param Form $form
     * @return HTTPResponse|DBHTMLText
     */
    public function doCompare($data, $form)

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

        $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(
                static::class . '_EditForm',
                'LeftAndMain_Content'
            ));
        }

        // non javascript, redirect the user to the page
        return $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 DBHTMLText|HTTPResponse
     */
    public function doShowVersion($data, $form)

    {
        $versionID = null;

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

        if (!$versionID) {
            return null;
        }

        $request = $this->getRequest();
        if ($request->isAjax()) {

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

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

    /**
     * @param int|null $versionID
     * @return Form
     */
    public function ShowVersionForm($versionID = null)
    {
        if (!$versionID) {
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
            return null;
        }

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

        return $form;
    }

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

        if (!$toVersion || !$fromVersion) {
            return null;
        }

        $id = $this->currentPageID();
        /** @var SiteTree $page */
        $page = SiteTree::get()->byID($id);

        $record = null;
        if ($page && $page->exists()) {
            if (!$page->canView()) {
                return Security::permissionFailure($this);
            }

            $record = $page->compareVersions($fromVersion, $toVersion);
class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
        }

        $fromVersionRecord = Versioned::get_version('SilverStripe\\CMS\\Model\\SiteTree', $id, $fromVersion);
        $toVersionRecord = Versioned::get_version('SilverStripe\\CMS\\Model\\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) {
            return null;
        }
        $form = $this->getEditForm($id, null, $fromVersion, $toVersion);
        $form->setActions(new FieldList());
        $form->addExtraClass('compare');

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

        // Comparison views shouldn't be editable.
        // As the comparison output is HTML and not valid values for the various field types
        $readonlyFields = $this->transformReadonly($form->Fields());
        $form->setFields($readonlyFields);

        return $form;
    }

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

    /**
     * Replace all data fields with HTML readonly fields to display diff
     *
     * @param FieldList $fields
     * @return FieldList
     */
    public function transformReadonly(FieldList $fields)
    {
        foreach ($fields->dataFields() as $field) {
            if ($field instanceof HiddenField) {
                continue;
            }
            $newField = $field->castedCopy(HTMLReadonlyField::class);
            $fields->replaceField($field->getName(), $newField);
        }
        return $fields;
    }
}


silverstripe / silverstripe-cms