Completed
Pull Request — master (#1536)
by Damian
02:49
created

CMSPageHistoryController::getEditForm()   C

Complexity

Conditions 9
Paths 28

Size

Total Lines 76
Code Lines 47

Duplication

Lines 0
Ratio 0 %

Importance

Changes 1
Bugs 0 Features 0
Metric Value
c 1
b 0
f 0
dl 0
loc 76
rs 5.8219
cc 9
eloc 47
nc 28
nop 4

How to fix   Long Method   

Long Method

Small methods make your code easier to understand, in particular if combined with a good name. Besides, if your method is small, finding a good name is usually much easier.

For example, if you find yourself adding comments to a method's body, this is usually a good sign to extract the commented part to a new method, and use the comment as a starting point when coming up with a good name for this new method.

Commonly applied refactorings include:

1
<?php
2
3
use SilverStripe\ORM\DataObject;
0 ignored issues
show
Bug introduced by
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 by
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
use SilverStripe\Security\Security;
0 ignored issues
show
Bug introduced by
This use statement conflicts with another class in this namespace, Security.

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...
6
7
/**
8
 * @package cms
9
 * @subpackage controllers
10
 */
11
class CMSPageHistoryController extends CMSMain {
12
13
	private static $url_segment = 'pages/history';
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
14
	private static $url_rule = '/$Action/$ID/$VersionID/$OtherVersionID';
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
15
	private static $url_priority = 42;
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
16
	private static $menu_title = 'History';
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
17
	private static $required_permission_codes = 'CMS_ACCESS_CMSMain';
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
18
	private static $session_namespace = 'CMSMain';
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
19
20
	private static $allowed_actions = array(
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
21
		'VersionsForm',
22
		'CompareVersionsForm',
23
		'show',
24
		'compare'
25
	);
26
27
	private static $url_handlers = array(
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
28
		'$Action/$ID/$VersionID/$OtherVersionID' => 'handleAction'
29
	);
30
31
	public function getResponseNegotiator() {
32
		$negotiator = parent::getResponseNegotiator();
33
		$controller = $this;
34
		$negotiator->setCallback('CurrentForm', function() use(&$controller) {
35
			$form = $controller->ShowVersionForm($controller->getRequest()->param('VersionID'));
36
			if($form) return $form->forTemplate();
37
			else return $controller->renderWith($controller->getTemplatesWithSuffix('_Content'));
38
		});
39
		$negotiator->setCallback('default', function() use(&$controller) {
40
			return $controller->renderWith($controller->getViewer('show'));
41
		});
42
		return $negotiator;
43
	}
44
45
	/**
46
	 * @param SS_HTTPRequest $request
47
	 * @return array
48
	 */
49 View Code Duplication
	public function show($request) {
0 ignored issues
show
Duplication introduced by
This method seems to be duplicated in your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
50
		$form = $this->ShowVersionForm($request->param('VersionID'));
51
52
		$negotiator = $this->getResponseNegotiator();
53
		$controller = $this;
54
		$negotiator->setCallback('CurrentForm', function() use(&$controller, &$form) {
55
			return $form ? $form->forTemplate() : $controller->renderWith($controller->getTemplatesWithSuffix('_Content'));
56
		});
57
		$negotiator->setCallback('default', function() use(&$controller, &$form) {
58
			return $controller->customise(array('EditForm' => $form))->renderWith($controller->getViewer('show'));
59
		});
60
61
		return $negotiator->respond($request);
62
	}
63
64
	/**
65
	 * @param SS_HTTPRequest $request
66
	 * @return array
67
	 */
68 View Code Duplication
	public function compare($request) {
0 ignored issues
show
Duplication introduced by
This method seems to be duplicated in your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
69
		$form = $this->CompareVersionsForm(
70
			$request->param('VersionID'),
71
			$request->param('OtherVersionID')
72
		);
73
74
		$negotiator = $this->getResponseNegotiator();
75
		$controller = $this;
76
		$negotiator->setCallback('CurrentForm', function() use(&$controller, &$form) {
77
			return $form ? $form->forTemplate() : $controller->renderWith($controller->getTemplatesWithSuffix('_Content'));
78
		});
79
		$negotiator->setCallback('default', function() use(&$controller, &$form) {
80
			return $controller->customise(array('EditForm' => $form))->renderWith($controller->getViewer('show'));
81
		});
82
83
		return $negotiator->respond($request);
84
	}
85
86
	public function getSilverStripeNavigator() {
87
		$record = $this->getRecord($this->currentPageID(), $this->getRequest()->param('VersionID'));
88
		if($record) {
89
			$navigator = new SilverStripeNavigator($record);
90
			return $navigator->renderWith($this->getTemplatesWithSuffix('_SilverStripeNavigator'));
0 ignored issues
show
Bug Best Practice introduced by
The return type of return $navigator->rende...lverStripeNavigator')); (SilverStripe\Model\FieldType\DBField) is incompatible with the return type of the parent method LeftAndMain::getSilverStripeNavigator of type ArrayData.

If you return a value from a function or method, it should be a sub-type of the type that is given by the parent type f.e. an interface, or abstract method. This is more formally defined by the Lizkov substitution principle, and guarantees that classes that depend on the parent type can use any instance of a child type interchangably. This principle also belongs to the SOLID principles for object oriented design.

Let’s take a look at an example:

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

Our function my_function expects a Post object, and outputs the author of the post. The base class Post returns a simple string and outputting a simple string will work just fine. However, the child class BlogPost which is a sub-type of Post instead decided to return an object, and is therefore violating the SOLID principles. If a BlogPost were passed to my_function, PHP would not complain, but ultimately fail when executing the strtoupper call in its body.

Loading history...
91
		} else {
92
			return false;
0 ignored issues
show
Bug Best Practice introduced by
The return type of return false; (false) is incompatible with the return type of the parent method LeftAndMain::getSilverStripeNavigator of type ArrayData.

If you return a value from a function or method, it should be a sub-type of the type that is given by the parent type f.e. an interface, or abstract method. This is more formally defined by the Lizkov substitution principle, and guarantees that classes that depend on the parent type can use any instance of a child type interchangably. This principle also belongs to the SOLID principles for object oriented design.

Let’s take a look at an example:

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

Our function my_function expects a Post object, and outputs the author of the post. The base class Post returns a simple string and outputting a simple string will work just fine. However, the child class BlogPost which is a sub-type of Post instead decided to return an object, and is therefore violating the SOLID principles. If a BlogPost were passed to my_function, PHP would not complain, but ultimately fail when executing the strtoupper call in its body.

Loading history...
93
		}
94
	}
95
96
	/**
97
	 * Returns the read only version of the edit form. Detaches all {@link FormAction}
98
	 * instances attached since only action relates to revert.
99
	 *
100
	 * Permission checking is done at the {@link CMSMain::getEditForm()} level.
101
	 *
102
	 * @param int $id ID of the record to show
103
	 * @param array $fields optional
104
	 * @param int $versionID
105
	 * @param int $compareID Compare mode
106
	 *
107
	 * @return Form
108
	 */
109
	public function getEditForm($id = null, $fields = null, $versionID = null, $compareID = null) {
110
		if(!$id) $id = $this->currentPageID();
0 ignored issues
show
Bug Best Practice introduced by
The expression $id of type integer|null is loosely compared to false; this is ambiguous if the integer can be zero. You might want to explicitly use === null instead.

In PHP, under loose comparison (like ==, or !=, or switch conditions), values of different types might be equal.

For integer values, zero is a special case, in particular the following results might be unexpected:

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
Loading history...
111
112
		$record = $this->getRecord($id, $versionID);
113
		$versionID = ($record) ? $record->Version : $versionID;
114
115
		$form = parent::getEditForm($record, ($record) ? $record->getCMSFields() : null);
116
		// Respect permission failures from parent implementation
117
		if(!($form instanceof Form)) return $form;
118
119
		// TODO: move to the SilverStripeNavigator structure so the new preview can pick it up.
120
		//$nav = new SilverStripeNavigatorItem_ArchiveLink($record);
121
122
		$form->setActions(new FieldList(
123
			$revert = FormAction::create('doRollback', _t('CMSPageHistoryController.REVERTTOTHISVERSION', 'Revert to this version'))->setUseButtonTag(true)
124
		));
125
126
		$fields = $form->Fields();
127
		$fields->removeByName("Status");
128
		$fields->push(new HiddenField("ID"));
129
		$fields->push(new HiddenField("Version"));
130
131
		$fields = $fields->makeReadonly();
132
133
		if($compareID) {
0 ignored issues
show
Bug Best Practice introduced by
The expression $compareID of type integer|null is loosely compared to true; this is ambiguous if the integer can be zero. You might want to explicitly use !== null instead.

In PHP, under loose comparison (like ==, or !=, or switch conditions), values of different types might be equal.

For integer values, zero is a special case, in particular the following results might be unexpected:

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
Loading history...
134
			$link = Controller::join_links(
135
				$this->Link('show'),
136
				$id
137
			);
138
139
			$view = _t('CMSPageHistoryController.VIEW',"view");
140
141
			$message = _t(
142
				'CMSPageHistoryController.COMPARINGVERSION',
143
				"Comparing versions {version1} and {version2}.",
144
				array(
0 ignored issues
show
Documentation introduced by
array('version1' => spri...k, $compareID), $view)) is of type array<string,string,{"ve...","version2":"string"}>, but the function expects a string.

It seems like the type of the argument is not accepted by the function/method which you are calling.

In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug.

We suggest to add an explicit type cast like in the following example:

function acceptsInteger($int) { }

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

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
Loading history...
145
					'version1' => sprintf('%s (<a href="%s">%s</a>)', $versionID, Controller::join_links($link, $versionID), $view),
146
					'version2' => sprintf('%s (<a href="%s">%s</a>)', $compareID, Controller::join_links($link, $compareID), $view)
147
				)
148
			);
149
150
			$revert->setReadonly(true);
151
		} else {
152
			if($record->isLatestVersion()) {
153
				$message = _t('CMSPageHistoryController.VIEWINGLATEST', 'Currently viewing the latest version.');
154
			} else {
155
				$message = _t(
156
					'CMSPageHistoryController.VIEWINGVERSION',
157
					"Currently viewing version {version}.",
158
					array('version' => $versionID)
0 ignored issues
show
Documentation introduced by
array('version' => $versionID) is of type array<string,?,{"version":"?"}>, but the function expects a string.

It seems like the type of the argument is not accepted by the function/method which you are calling.

In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug.

We suggest to add an explicit type cast like in the following example:

function acceptsInteger($int) { }

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

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
Loading history...
159
				);
160
			}
161
		}
162
163
		$fields->addFieldToTab('Root.Main',
164
			new LiteralField('CurrentlyViewingMessage', $this->customise(array(
165
				'Content' => $message,
166
				'Classes' => 'notice'
167
			))->renderWith(array('CMSMain_notice'))),
168
			"Title"
169
		);
170
171
		$form->setFields($fields->makeReadonly());
172
		$form->loadDataFrom(array(
173
			"ID" => $id,
174
			"Version" => $versionID,
175
		));
176
177
		if(($record && $record->isLatestVersion())) {
178
			$revert->setReadonly(true);
179
		}
180
181
		$form->removeExtraClass('cms-content');
182
183
		return $form;
184
	}
185
186
187
	/**
188
	 * Version select form. Main interface between selecting versions to view
189
	 * and comparing multiple versions.
190
	 *
191
	 * Because we can reload the page directly to a compare view (history/compare/1/2/3)
192
	 * this form has to adapt to those parameters as well.
193
	 *
194
	 * @return Form
195
	 */
196
	public function VersionsForm() {
197
		$id = $this->currentPageID();
198
		$page = $this->getRecord($id);
199
		$versionsHtml = '';
200
201
		$action = $this->getRequest()->param('Action');
202
		$versionID = $this->getRequest()->param('VersionID');
203
		$otherVersionID = $this->getRequest()->param('OtherVersionID');
204
205
		$showUnpublishedChecked = 0;
206
		$compareModeChecked = ($action == "compare");
207
208
		if($page) {
209
			$versions = $page->allVersions();
210
			$versionID = (!$versionID) ? $page->Version : $versionID;
211
212
			if($versions) {
213
				foreach($versions as $k => $version) {
214
					$active = false;
215
216
					if($version->Version == $versionID || $version->Version == $otherVersionID) {
217
						$active = true;
218
219
						if(!$version->WasPublished) $showUnpublishedChecked = 1;
220
					}
221
222
					$version->Active = ($active);
223
				}
224
			}
225
226
			$vd = new ViewableData();
227
228
			$versionsHtml = $vd->customise(array(
229
				'Versions' => $versions
230
			))->renderWith('CMSPageHistoryController_versions');
231
		}
232
233
		$fields = new FieldList(
234
			new CheckboxField(
235
				'ShowUnpublished',
236
				_t('CMSPageHistoryController.SHOWUNPUBLISHED','Show unpublished versions'),
237
				$showUnpublishedChecked
238
			),
239
			new CheckboxField(
240
				'CompareMode',
241
				_t('CMSPageHistoryController.COMPAREMODE', 'Compare mode (select two)'),
242
				$compareModeChecked
243
			),
244
			new LiteralField('VersionsHtml', $versionsHtml),
0 ignored issues
show
Bug introduced by
It seems like $versionsHtml defined by $vd->customise(array('Ve...ryController_versions') on line 228 can also be of type object<SilverStripe\Model\FieldType\DBField>; however, LiteralField::__construct() does only seem to accept string|object<FormField>, maybe add an additional type check?

If a method or function can return multiple different values and unless you are sure that you only can receive a single value in this context, we recommend to add an additional type check:

/**
 * @return array|string
 */
function returnsDifferentValues($x) {
    if ($x) {
        return 'foo';
    }

    return array();
}

$x = returnsDifferentValues($y);
if (is_array($x)) {
    // $x is an array.
}

If this a common case that PHP Analyzer should handle natively, please let us know by opening an issue.

Loading history...
245
			$hiddenID = new HiddenField('ID', false, "")
0 ignored issues
show
Documentation introduced by
false is of type boolean, but the function expects a null|string.

It seems like the type of the argument is not accepted by the function/method which you are calling.

In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug.

We suggest to add an explicit type cast like in the following example:

function acceptsInteger($int) { }

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

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
Loading history...
246
		);
247
248
		$actions = new FieldList(
249
			new FormAction(
250
				'doCompare', _t('CMSPageHistoryController.COMPAREVERSIONS','Compare Versions')
251
			),
252
			new FormAction(
253
				'doShowVersion', _t('CMSPageHistoryController.SHOWVERSION','Show Version')
254
			)
255
		);
256
257
		// Use <button> to allow full jQuery UI styling
258
		foreach($actions->dataFields() as $action) $action->setUseButtonTag(true);
259
260
		$negotiator = $this->getResponseNegotiator();
0 ignored issues
show
Unused Code introduced by
$negotiator is not used, you could remove the assignment.

This check looks for variable assignements that are either overwritten by other assignments or where the variable is not used subsequently.

$myVar = 'Value';
$higher = false;

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

Both the $myVar assignment in line 1 and the $higher assignment in line 2 are dead. The first because $myVar is never used and the second because $higher is always overwritten for every possible time line.

Loading history...
261
		$form = Form::create(
262
			$this,
263
			'VersionsForm',
264
			$fields,
265
			$actions
266
		)->setHTMLID('Form_VersionsForm');
267
		$form->loadDataFrom($this->getRequest()->requestVars());
268
		$hiddenID->setValue($id);
269
		$form->unsetValidator();
270
271
		$form
272
			->addExtraClass('cms-versions-form') // placeholder, necessary for $.metadata() to work
273
			->setAttribute('data-link-tmpl-compare', Controller::join_links($this->Link('compare'), '%s', '%s', '%s'))
274
			->setAttribute('data-link-tmpl-show', Controller::join_links($this->Link('show'), '%s', '%s'));
275
276
		return $form;
277
	}
278
279
	/**
280
	 * Process the {@link VersionsForm} compare function between two pages.
281
	 *
282
	 * @param array
283
	 * @param Form
284
	 *
285
	 * @return html
286
	 */
287
	public function doCompare($data, $form) {
288
		$versions = $data['Versions'];
289
		if(count($versions) < 2) return null;
290
291
		$id = $this->currentPageID();
0 ignored issues
show
Unused Code introduced by
$id is not used, you could remove the assignment.

This check looks for variable assignements that are either overwritten by other assignments or where the variable is not used subsequently.

$myVar = 'Value';
$higher = false;

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

Both the $myVar assignment in line 1 and the $higher assignment in line 2 are dead. The first because $myVar is never used and the second because $higher is always overwritten for every possible time line.

Loading history...
292
		$version1 = array_shift($versions);
293
		$version2 = array_shift($versions);
294
295
		$form = $this->CompareVersionsForm($version1, $version2);
296
297
		// javascript solution, render into template
298 View Code Duplication
		if($this->getRequest()->isAjax()) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
299
			return $this->customise(array(
300
				"EditForm" => $form
301
			))->renderWith(array(
302
				$this->class . '_EditForm',
303
				'LeftAndMain_Content'
304
			));
305
		}
306
307
		// non javascript, redirect the user to the page
308
		$this->redirect(Controller::join_links(
309
			$this->Link('compare'),
310
			$version1,
311
			$version2
312
		));
313
	}
314
315
	/**
316
	 * Process the {@link VersionsForm} show version function. Only requires
317
	 * one page to be selected.
318
	 *
319
	 * @param array
320
	 * @param Form
321
	 *
322
	 * @return html
323
	 */
324
	public function doShowVersion($data, $form) {
325
		$versionID = null;
326
327
		if(isset($data['Versions']) && is_array($data['Versions'])) {
328
			$versionID  = array_shift($data['Versions']);
329
		}
330
331
		if(!$versionID) return;
332
333 View Code Duplication
		if($request->isAjax()) {
0 ignored issues
show
Bug introduced by
The variable $request does not exist. Did you forget to declare it?

This check marks access to variables or properties that have not been declared yet. While PHP has no explicit notion of declaring a variable, accessing it before a value is assigned to it is most likely a bug.

Loading history...
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
334
			return $this->customise(array(
335
				"EditForm" => $this->ShowVersionForm($versionID)
336
			))->renderWith(array(
337
				$this->class . '_EditForm',
338
				'LeftAndMain_Content'
339
			));
340
		}
341
342
		// non javascript, redirect the user to the page
343
		$this->redirect(Controller::join_links(
344
			$this->Link('version'),
345
			$versionID
346
		));
347
	}
348
349
	/**
350
	 * @param int|null $versionID
351
	 * @return Form
352
	 */
353
	public function ShowVersionForm($versionID = null) {
354
		if(!$versionID) return null;
0 ignored issues
show
Bug Best Practice introduced by
The expression $versionID of type integer|null is loosely compared to false; this is ambiguous if the integer can be zero. You might want to explicitly use === null instead.

In PHP, under loose comparison (like ==, or !=, or switch conditions), values of different types might be equal.

For integer values, zero is a special case, in particular the following results might be unexpected:

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
Loading history...
355
356
		$id = $this->currentPageID();
357
		$form = $this->getEditForm($id, null, $versionID);
358
359
		return $form;
360
	}
361
362
	/**
363
	 * @param int $versionID
364
	 * @param int $otherVersionID
365
	 * @return Form
366
	 */
367
	public function CompareVersionsForm($versionID, $otherVersionID) {
368
		if($versionID > $otherVersionID) {
369
			$toVersion = $versionID;
370
			$fromVersion = $otherVersionID;
371
		} else {
372
			$toVersion = $otherVersionID;
373
			$fromVersion = $versionID;
374
		}
375
376
		if(!$toVersion || !$fromVersion) return false;
0 ignored issues
show
Bug Best Practice introduced by
The return type of return false; (false) is incompatible with the return type documented by CMSPageHistoryController::CompareVersionsForm of type Form.

If you return a value from a function or method, it should be a sub-type of the type that is given by the parent type f.e. an interface, or abstract method. This is more formally defined by the Lizkov substitution principle, and guarantees that classes that depend on the parent type can use any instance of a child type interchangably. This principle also belongs to the SOLID principles for object oriented design.

Let’s take a look at an example:

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

Our function my_function expects a Post object, and outputs the author of the post. The base class Post returns a simple string and outputting a simple string will work just fine. However, the child class BlogPost which is a sub-type of Post instead decided to return an object, and is therefore violating the SOLID principles. If a BlogPost were passed to my_function, PHP would not complain, but ultimately fail when executing the strtoupper call in its body.

Loading history...
377
378
		$id = $this->currentPageID();
379
		$page = DataObject::get_by_id("SiteTree", $id);
380
381
		if($page && !$page->canView()) {
382
			return Security::permissionFailure($this);
383
		}
384
385
		$record = $page->compareVersions($fromVersion, $toVersion);
386
387
		$fromVersionRecord = Versioned::get_version('SiteTree', $id, $fromVersion);
388
		$toVersionRecord = Versioned::get_version('SiteTree', $id, $toVersion);
389
390
		if(!$fromVersionRecord) {
391
			user_error("Can't find version $fromVersion of page $id", E_USER_ERROR);
392
		}
393
394
		if(!$toVersionRecord) {
395
			user_error("Can't find version $toVersion of page $id", E_USER_ERROR);
396
		}
397
398
		if($record) {
399
			$form = $this->getEditForm($id, null, null, true);
400
			$form->setActions(new FieldList());
401
			$form->addExtraClass('compare');
402
403
			// Comparison views shouldn't be editable.
404
			// Its important to convert fields *before* loading data,
405
			// as the comparison output is HTML and not valid values for the various field types
406
			$readonlyFields = $form->Fields()->makeReadonly();
407
			$form->setFields($readonlyFields);
408
409
			$form->loadDataFrom($record);
410
			$form->loadDataFrom(array(
411
				"ID" => $id,
412
				"Version" => $fromVersion,
413
			));
414
415
			foreach($form->Fields()->dataFields() as $field) {
416
				$field->dontEscape = true;
417
			}
418
419
			return $form;
420
		}
421
	}
422
423
	public function Breadcrumbs($unlinked = false) {
424
		$crumbs = parent::Breadcrumbs($unlinked);
425
		$crumbs[0]->Title = _t('CMSPagesController.MENUTITLE');
426
		return $crumbs;
427
	}
428
429
}
430