Completed
Push — master ( 25a8fb...29fc7a )
by Hamish
12s
created

SearchForm   B

Complexity

Total Complexity 40

Size/Duplication

Total Lines 210
Duplicated Lines 9.52 %

Coupling/Cohesion

Components 1
Dependencies 7

Importance

Changes 1
Bugs 0 Features 0
Metric Value
wmc 40
lcom 1
cbo 7
dl 20
loc 210
rs 8.2608
c 1
b 0
f 0

9 Methods

Rating   Name   Duplication   Size   Complexity  
B __construct() 0 23 5
A forTemplate() 0 11 1
A classesToSearch() 0 8 2
A getClassesToSearch() 0 3 1
D getResults() 20 60 20
B addStarsToKeywords() 0 17 6
A getSearchQuery() 0 7 3
A setPageLength() 0 3 1
A getPageLength() 0 3 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:

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 SearchForm 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 SearchForm, and based on these observations, apply Extract Interface, too.

1
<?php
2
3
namespace SilverStripe\CMS\Search;
4
5
use Controller;
6
use SilverStripe\ORM\DB;
7
use SilverStripe\ORM\SS_List;
8
use Form;
9
use FieldList;
10
use TextField;
11
use HiddenField;
12
use Translatable;
13
use FormAction;
14
15
/**
16
 * Standard basic search form which conducts a fulltext search on all {@link SiteTree}
17
 * objects.
18
 *
19
 * If multilingual content is enabled through the {@link Translatable} extension,
20
 * only pages the currently set language on the holder for this searchform are found.
21
 * The language is set through a hidden field in the form, which is prepoluated
22
 * with {@link Translatable::get_current_locale()} when then form is constructed.
23
 *
24
 * @see Use ModelController and SearchContext for a more generic search implementation based around DataObject
25
 * @package cms
26
 * @subpackage search
27
 */
28
class SearchForm extends Form {
29
30
	/**
31
	 * @var int $pageLength How many results are shown per page.
32
	 * Relies on pagination being implemented in the search results template.
33
	 */
34
	protected $pageLength = 10;
35
36
	/**
37
	 * Classes to search
38
	 */
39
 	protected $classesToSearch = array(
40
		"SilverStripe\\CMS\\Model\\SiteTree", "File"
41
	);
42
43
	private static $casting = 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...
44
		'SearchQuery' => 'Text'
45
	);
46
47
	/**
48
	 *
49
	 * @param Controller $controller
50
	 * @param string $name The name of the form (used in URL addressing)
51
	 * @param FieldList $fields Optional, defaults to a single field named "Search". Search logic needs to be customized
52
	 *  if fields are added to the form.
53
	 * @param FieldList $actions Optional, defaults to a single field named "Go".
54
	 */
55
	public function __construct($controller, $name, $fields = null, $actions = null) {
56
		if(!$fields) {
57
			$fields = new FieldList(
58
				new TextField('Search', _t('SearchForm.SEARCH', 'Search')
59
			));
60
		}
61
62
		if(class_exists('Translatable') && singleton('SilverStripe\\CMS\\Model\\SiteTree')->hasExtension('Translatable')) {
63
			$fields->push(new HiddenField('searchlocale', 'searchlocale', Translatable::get_current_locale()));
64
		}
65
66
		if(!$actions) {
67
			$actions = new FieldList(
68
				new FormAction("getResults", _t('SearchForm.GO', 'Go'))
69
			);
70
		}
71
72
		parent::__construct($controller, $name, $fields, $actions);
73
74
		$this->setFormMethod('get');
75
76
		$this->disableSecurityToken();
77
	}
78
79
80
	/**
81
	 * Return a rendered version of this form.
82
	 *
83
	 * This is returned when you access a form as $FormObject rather
84
	 * than <% with FormObject %>
85
	 */
86
	public function forTemplate() {
87
		$return = $this->renderWith(array_merge(
88
			(array)$this->getTemplate(),
89
			array('SilverStripe\\CMS\\Search\\SearchForm', 'Form')
90
		));
91
92
		// Now that we're rendered, clear message
93
		$this->clearMessage();
94
95
		return $return;
96
	}
97
98
	/**
99
	 * Set the classes to search.
100
	 * Currently you can only choose from "SiteTree" and "File", but a future version might improve this.
101
 	 */
102
	public function classesToSearch($classes) {
103
		$illegalClasses = array_diff($classes, array('SilverStripe\\CMS\\Model\\SiteTree', 'File'));
104
		if($illegalClasses) {
0 ignored issues
show
Bug Best Practice introduced by
The expression $illegalClasses of type array is implicitly converted to a boolean; are you sure this is intended? If so, consider using ! empty($expr) instead to make it clear that you intend to check for an array without elements.

This check marks implicit conversions of arrays to boolean values in a comparison. While in PHP an empty array is considered to be equal (but not identical) to false, this is not always apparent.

Consider making the comparison explicit by using empty(..) or ! empty(...) instead.

Loading history...
105
			user_error("SearchForm::classesToSearch() passed illegal classes '" . implode("', '", $illegalClasses) . "'.  At this stage, only File and SiteTree are allowed", E_USER_WARNING);
106
		}
107
		$legalClasses = array_intersect($classes, array('SilverStripe\\CMS\\Model\\SiteTree', 'File'));
108
		$this->classesToSearch = $legalClasses;
109
	}
110
111
	/**
112
	 * Get the classes to search
113
	 *
114
	 * @return array
115
	 */
116
	public function getClassesToSearch() {
117
		return $this->classesToSearch;
118
	}
119
120
	/**
121
	 * Return dataObjectSet of the results using $_REQUEST to get info from form.
122
	 * Wraps around {@link searchEngine()}.
123
	 *
124
	 * @param int $pageLength DEPRECATED 2.3 Use SearchForm->pageLength
125
	 * @param array $data Request data as an associative array. Should contain at least a key 'Search' with all searched keywords.
126
	 * @return SS_List
127
	 */
128
	public function getResults($pageLength = null, $data = null){
129
	 	// legacy usage: $data was defaulting to $_REQUEST, parameter not passed in doc.silverstripe.org tutorials
130
		if(!isset($data) || !is_array($data)) $data = $_REQUEST;
131
132
		// set language (if present)
133 View Code Duplication
		if(class_exists('Translatable')) {
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...
134
			if(singleton('SilverStripe\\CMS\\Model\\SiteTree')->hasExtension('Translatable') && isset($data['searchlocale'])) {
135
				if($data['searchlocale'] == "ALL") {
136
					Translatable::disable_locale_filter();
137
				} else {
138
					$origLocale = Translatable::get_current_locale();
139
140
					Translatable::set_current_locale($data['searchlocale']);
141
				}
142
			}
143
		}
144
145
		$keywords = $data['Search'];
146
147
	 	$andProcessor = create_function('$matches','
148
	 		return " +" . $matches[2] . " +" . $matches[4] . " ";
149
	 	');
150
	 	$notProcessor = create_function('$matches', '
151
	 		return " -" . $matches[3];
152
	 	');
153
154
	 	$keywords = preg_replace_callback('/()("[^()"]+")( and )("[^"()]+")()/i', $andProcessor, $keywords);
155
	 	$keywords = preg_replace_callback('/(^| )([^() ]+)( and )([^ ()]+)( |$)/i', $andProcessor, $keywords);
156
		$keywords = preg_replace_callback('/(^| )(not )("[^"()]+")/i', $notProcessor, $keywords);
157
		$keywords = preg_replace_callback('/(^| )(not )([^() ]+)( |$)/i', $notProcessor, $keywords);
158
159
		$keywords = $this->addStarsToKeywords($keywords);
160
161
		if(!$pageLength) $pageLength = $this->pageLength;
0 ignored issues
show
Bug Best Practice introduced by
The expression $pageLength 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...
162
		$start = isset($_GET['start']) ? (int)$_GET['start'] : 0;
163
164
		if(strpos($keywords, '"') !== false || strpos($keywords, '+') !== false || strpos($keywords, '-') !== false || strpos($keywords, '*') !== false) {
165
			$results = DB::get_conn()->searchEngine($this->classesToSearch, $keywords, $start, $pageLength, "\"Relevance\" DESC", "", true);
166
		} else {
167
			$results = DB::get_conn()->searchEngine($this->classesToSearch, $keywords, $start, $pageLength);
168
		}
169
170
		// filter by permission
171
		if($results) foreach($results as $result) {
172
			if(!$result->canView()) $results->remove($result);
173
		}
174
175
		// reset locale
176 View Code Duplication
		if(class_exists('Translatable')) {
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...
177
			if(singleton('SilverStripe\\CMS\\Model\\SiteTree')->hasExtension('Translatable') && isset($data['searchlocale'])) {
178
				if($data['searchlocale'] == "ALL") {
179
					Translatable::enable_locale_filter();
180
				} else {
181
					Translatable::set_current_locale($origLocale);
0 ignored issues
show
Bug introduced by
The variable $origLocale does not seem to be defined for all execution paths leading up to this point.

If you define a variable conditionally, it can happen that it is not defined for all execution paths.

Let’s take a look at an example:

function myFunction($a) {
    switch ($a) {
        case 'foo':
            $x = 1;
            break;

        case 'bar':
            $x = 2;
            break;
    }

    // $x is potentially undefined here.
    echo $x;
}

In the above example, the variable $x is defined if you pass “foo” or “bar” as argument for $a. However, since the switch statement has no default case statement, if you pass any other value, the variable $x would be undefined.

Available Fixes

  1. Check for existence of the variable explicitly:

    function myFunction($a) {
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
        }
    
        if (isset($x)) { // Make sure it's always set.
            echo $x;
        }
    }
    
  2. Define a default value for the variable:

    function myFunction($a) {
        $x = ''; // Set a default which gets overridden for certain paths.
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
        }
    
        echo $x;
    }
    
  3. Add a value for the missing path:

    function myFunction($a) {
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
    
            // We add support for the missing case.
            default:
                $x = '';
                break;
        }
    
        echo $x;
    }
    
Loading history...
182
				}
183
			}
184
		}
185
186
		return $results;
187
	}
188
189
	protected function addStarsToKeywords($keywords) {
190
		if(!trim($keywords)) return "";
191
		// Add * to each keyword
192
		$splitWords = preg_split("/ +/" , trim($keywords));
193
		while(list($i,$word) = each($splitWords)) {
0 ignored issues
show
Unused Code introduced by
The assignment to $i is unused. Consider omitting it like so list($first,,$third).

This checks looks for assignemnts to variables using the list(...) function, where not all assigned variables are subsequently used.

Consider the following code example.

<?php

function returnThreeValues() {
    return array('a', 'b', 'c');
}

list($a, $b, $c) = returnThreeValues();

print $a . " - " . $c;

Only the variables $a and $c are used. There was no need to assign $b.

Instead, the list call could have been.

list($a,, $c) = returnThreeValues();
Loading history...
194
			if($word[0] == '"') {
195
				while(list($i,$subword) = each($splitWords)) {
0 ignored issues
show
Unused Code introduced by
The assignment to $i is unused. Consider omitting it like so list($first,,$third).

This checks looks for assignemnts to variables using the list(...) function, where not all assigned variables are subsequently used.

Consider the following code example.

<?php

function returnThreeValues() {
    return array('a', 'b', 'c');
}

list($a, $b, $c) = returnThreeValues();

print $a . " - " . $c;

Only the variables $a and $c are used. There was no need to assign $b.

Instead, the list call could have been.

list($a,, $c) = returnThreeValues();
Loading history...
196
					$word .= ' ' . $subword;
197
					if(substr($subword,-1) == '"') break;
198
				}
199
			} else {
200
				$word .= '*';
201
			}
202
			$newWords[] = $word;
0 ignored issues
show
Coding Style Comprehensibility introduced by
$newWords was never initialized. Although not strictly required by PHP, it is generally a good practice to add $newWords = array(); before regardless.

Adding an explicit array definition is generally preferable to implicit array definition as it guarantees a stable state of the code.

Let’s take a look at an example:

foreach ($collection as $item) {
    $myArray['foo'] = $item->getFoo();

    if ($item->hasBar()) {
        $myArray['bar'] = $item->getBar();
    }

    // do something with $myArray
}

As you can see in this example, the array $myArray is initialized the first time when the foreach loop is entered. You can also see that the value of the bar key is only written conditionally; thus, its value might result from a previous iteration.

This might or might not be intended. To make your intention clear, your code more readible and to avoid accidental bugs, we recommend to add an explicit initialization $myArray = array() either outside or inside the foreach loop.

Loading history...
203
		}
204
		return implode(" ", $newWords);
0 ignored issues
show
Bug introduced by
The variable $newWords does not seem to be defined for all execution paths leading up to this point.

If you define a variable conditionally, it can happen that it is not defined for all execution paths.

Let’s take a look at an example:

function myFunction($a) {
    switch ($a) {
        case 'foo':
            $x = 1;
            break;

        case 'bar':
            $x = 2;
            break;
    }

    // $x is potentially undefined here.
    echo $x;
}

In the above example, the variable $x is defined if you pass “foo” or “bar” as argument for $a. However, since the switch statement has no default case statement, if you pass any other value, the variable $x would be undefined.

Available Fixes

  1. Check for existence of the variable explicitly:

    function myFunction($a) {
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
        }
    
        if (isset($x)) { // Make sure it's always set.
            echo $x;
        }
    }
    
  2. Define a default value for the variable:

    function myFunction($a) {
        $x = ''; // Set a default which gets overridden for certain paths.
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
        }
    
        echo $x;
    }
    
  3. Add a value for the missing path:

    function myFunction($a) {
        switch ($a) {
            case 'foo':
                $x = 1;
                break;
    
            case 'bar':
                $x = 2;
                break;
    
            // We add support for the missing case.
            default:
                $x = '';
                break;
        }
    
        echo $x;
    }
    
Loading history...
205
	}
206
207
	/**
208
	 * Get the search query for display in a "You searched for ..." sentence.
209
	 *
210
	 * @param array $data
211
	 * @return string
212
	 */
213
	public function getSearchQuery($data = null) {
214
		// legacy usage: $data was defaulting to $_REQUEST, parameter not passed in doc.silverstripe.org tutorials
215
		if(!isset($data)) $data = $_REQUEST;
216
217
		// The form could be rendered without the search being done, so check for that.
218
		if (isset($data['Search'])) return $data['Search'];
219
	}
220
221
	/**
222
	 * Set the maximum number of records shown on each page.
223
	 *
224
	 * @param int $length
225
	 */
226
	public function setPageLength($length) {
227
		$this->pageLength = $length;
228
	}
229
230
	/**
231
	 * @return int
232
	 */
233
	public function getPageLength() {
234
		return $this->pageLength;
235
	}
236
237
}
238
239
240