Skip to content

Commit a01e853

Browse files
b3nlb3nl
authored
Merge pull request #79 from bestit/feature/PHPCS-247-warn-against-type-methods
PHPCS-247 Added warning against simple getter and setter
2 parents 7361697 + 453a4b0 commit a01e853

File tree

9 files changed

+429
-10
lines changed

9 files changed

+429
-10
lines changed

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,7 @@ The base for the BestIt Standard is [PSR-12](https://github.com/php-fig/fig-stan
157157
| BestIt.Functions.ForbiddenFunctions.Discouraged | You SHOULD not use eval. | yes |
158158
| BestIt.Functions.ForbiddenFunctions.DiscouragedWithAlternative | You SHOULD not use alias but the original function names. | yes |
159159
| BestIt.Functions.MultipleReturn.MultipleReturnsFound | You SHOULD only use a return per method. | no |
160+
| BestIt.Functions.NoSimplePropertyMethod.ShouldUseTypedPropertyDirectly | You SHOULD use the typed property directly instead of the the simple getter-/setter-combination. | yes |
160161
| BestIt.Functions.TrailingCommaInCall.MissingTrailingComma | You MUST append a trailing command in your multi line function calls. | no |
161162
| BestIt.NamingConventions.CamelCaseVariable.NotCamelCase | You MUST provide your vars in camel case, lower case first. | yes |
162163
| BestIt.TypeHints.ExplicitAssertions.RequiredExplicitAssertion | Use assertion instead of inline documentation comment. | no |

‎phpmd.xml.dist‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,6 @@
3434

3535
<rule ref="rulesets/controversial.xml"/>
3636

37-
<!-- There seemed to be a phpmd bug which complained about AbstractSniff without change or real existing error -->
38-
<rule ref="rulesets/design.xml/NumberOfChildren">
39-
<properties>
40-
<property name="minimum" value="20"/>
41-
</properties>
42-
</rule>
43-
4437
<rule ref="rulesets/design.xml">
4538
<exclude name="NumberOfChildren" />
4639
</rule>

‎src/Standards/BestIt/Sniffs/Functions/FluentSetterSniff.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ private function checkIfSetterFunction(int $classPosition, File $file, int $meth
202202
);
203203

204204
// We require camelCase for methods and properties,
205-
// so there should be an "lcfirst-Method" without set-prefix.
205+
// so there should be an "lcfirst-property" without set-prefix.
206206
$isSetter = in_array(lcfirst(substr($methodName, 3)), $properties, true);
207207
}
208208

‎src/Standards/BestIt/Sniffs/Functions/NoSimplePropertyMethodSniff.php‎

Lines changed: 264 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,264 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace BestIt\Sniffs\Functions;
6+
7+
use BestIt\Sniffs\AbstractSniff;
8+
use BestIt\Sniffs\SuppressingTrait;
9+
use SlevomatCodingStandard\Helpers\FunctionHelper;
10+
use SlevomatCodingStandard\Helpers\PropertyHelper;
11+
use SlevomatCodingStandard\Helpers\TokenHelper;
12+
use function current;
13+
use function is_int;
14+
use function preg_match;
15+
use function substr;
16+
use function trim;
17+
use function ucfirst;
18+
use const T_AS;
19+
use const T_CONST;
20+
use const T_FUNCTION;
21+
use const T_PRIVATE;
22+
use const T_PROTECTED;
23+
use const T_PUBLIC;
24+
use const T_STRING;
25+
use const T_VAR;
26+
use const T_VARIABLE;
27+
28+
/**
29+
* We want to encourage that you use the new typed properties instead of redundant simple getter and setters.
30+
*
31+
* @author blange <[email protected]>
32+
* @package BestIt\Sniffs\Functions
33+
*/
34+
class NoSimplePropertyMethodSniff extends AbstractSniff
35+
{
36+
use SuppressingTrait;
37+
38+
/**
39+
* The error code for the getter.
40+
*
41+
* @var string
42+
*/
43+
public const CODE_SHOULD_USE_PROPERTY = 'ShouldUseTypedPropertyDirectly';
44+
45+
/**
46+
* Error message when a simple getter exists.
47+
*
48+
* @var string
49+
*/
50+
private const ERROR_GETTER_TOO_SIMPLE = 'We suggest that you use the typed property "%s" directly.';
51+
52+
/**
53+
* Error message when a simple getter exists.
54+
*
55+
* @var string
56+
*/
57+
private const ERROR_SETTER_TOO_SIMPLE = 'We suggest that you use the typed property "%s" directly.';
58+
59+
/**
60+
* Method getter for the setter methods.
61+
*
62+
* @var string
63+
*/
64+
private const METHOD_PREFIX_GETTER = 'get';
65+
66+
/**
67+
* Method getter for the setter methods.
68+
*
69+
* @var string
70+
*/
71+
private const METHOD_PREFIX_SETTER = 'set';
72+
73+
/**
74+
* Finds the position of the method token which is named like the given string.
75+
*
76+
* @param string $string
77+
*
78+
* @return int|null
79+
*/
80+
private function findMethod(string $string): ?int
81+
{
82+
$file = $this->getFile();
83+
$searchPos = $this->getStackPos();
84+
85+
do {
86+
$searchPos = $file->findNext(T_FUNCTION, ++$searchPos);
87+
88+
$nextPos = $searchPos + 1;
89+
$possibleNextName = $nextPos + 2;
90+
} while ($searchPos && !$file->findNext(T_STRING, $nextPos, $possibleNextName, false, $string));
91+
92+
return is_int($searchPos) ? $searchPos : null;
93+
}
94+
95+
/**
96+
* Returns the stack position of the relevant property if it is a typed one.
97+
*
98+
* @return int|null
99+
*/
100+
private function getExactPropertyPointer(): ?int
101+
{
102+
$asPointer = TokenHelper::findPreviousEffective($this->getFile(), $this->getStackPos() - 1);
103+
104+
if ($this->tokens[$asPointer]['code'] === T_AS) {
105+
return null;
106+
}
107+
108+
$propertyPointer = TokenHelper::findNext(
109+
$this->getFile(),
110+
[T_FUNCTION, T_CONST, T_VARIABLE],
111+
$this->getStackPos() + 1,
112+
);
113+
114+
if ($this->tokens[$propertyPointer]['code'] !== T_VARIABLE) {
115+
return null;
116+
}
117+
118+
$propertyTypeHint = PropertyHelper::findTypeHint($this->getFile(), $propertyPointer);
119+
120+
if (!$propertyTypeHint) {
121+
return null;
122+
}
123+
124+
return $propertyPointer;
125+
}
126+
127+
/**
128+
* Returns true if this sniff or a rule of this sniff is suppressed with the slevomat suppress annotation.
129+
*
130+
* @param null|string $rule The optional rule.
131+
* @param int|null $stackPos Do you want ot overload the position for the which position the sniff is suppressed.
132+
*
133+
* @return bool Returns true if the sniff is suppressed.
134+
*/
135+
protected function isSniffSuppressed(?string $rule = null, ?int $stackPos = null): bool
136+
{
137+
return $this->getSuppressHelper()->isSniffSuppressed(
138+
$this->getFile(),
139+
$stackPos ?? $this->getStackPos(),
140+
$this->getSniffName($rule),
141+
);
142+
}
143+
144+
/**
145+
* Returns true if the getter is too simple and does not provide more functionality.
146+
*
147+
* @param string $propertyName
148+
* @param int $methodPosition
149+
*
150+
* @return bool
151+
*/
152+
private function isTooSimpleGetter(string $propertyName, int $methodPosition): bool
153+
{
154+
return $this->matchesMethodContentToRegex(
155+
'/return \$this->' . $propertyName . '\s*;$/m',
156+
$methodPosition,
157+
);
158+
}
159+
160+
/**
161+
* Returns true if the setter is too simple and does not provide more functionality.
162+
*
163+
* @param string $propertyName
164+
* @param int $methodPosition
165+
*
166+
* @return bool
167+
*/
168+
private function isTooSimpleSetter(string $propertyName, int $methodPosition): bool
169+
{
170+
$isTooSimpleSetter = false;
171+
$file = $this->getFile();
172+
173+
$parameters = FunctionHelper::getParametersNames($file, $methodPosition);
174+
175+
if (count($parameters) === 1) {
176+
$parameterName = current($parameters);
177+
178+
$isTooSimpleSetter = $this->matchesMethodContentToRegex(
179+
'/\$this->' . $propertyName . '\s*=\s*\\' . $parameterName . '\s*;' .
180+
// finish with the assignment or wait for the fluent return.
181+
'($|\s*return\s*\$this\s*;$)/m',
182+
$methodPosition,
183+
);
184+
}
185+
186+
return $isTooSimpleSetter;
187+
}
188+
189+
/**
190+
* Returns true if the methods content matches the given regex.
191+
*
192+
* @param string $contentRegex
193+
*
194+
* @return bool
195+
*/
196+
private function matchesMethodContentToRegex(string $contentRegex, int $methodPosition): bool
197+
{
198+
$matches = false;
199+
$methodToken = $this->tokens[$methodPosition];
200+
201+
if ($methodToken && @$methodToken['scope_opener'] && @$methodToken['scope_closer']) {
202+
$fileContent = trim($this->getFile()->getTokensAsString(
203+
$methodToken['scope_opener'] + 1,
204+
$methodToken['scope_closer'] - $methodToken['scope_opener'] - 1,
205+
));
206+
207+
$matches = (bool) preg_match($contentRegex, $fileContent);
208+
}
209+
210+
return $matches;
211+
}
212+
213+
/**
214+
* Checks if the sniff on a typed property is not suppressed to check for a simple getter and setter.
215+
*
216+
* @return void
217+
*/
218+
protected function processToken(): void
219+
{
220+
$propertyPointer = $this->getExactPropertyPointer();
221+
222+
if (!($propertyPointer && !$this->isSniffSuppressed(static::CODE_SHOULD_USE_PROPERTY))) {
223+
return;
224+
}
225+
226+
$propertyName = substr($this->tokens[$propertyPointer]['content'], 1);
227+
$setterPosition = $this->findMethod(self::METHOD_PREFIX_SETTER . ucfirst($propertyName));
228+
$getterPosition = $this->findMethod(self::METHOD_PREFIX_GETTER . ucfirst($propertyName));
229+
230+
if (
231+
$getterPosition && $setterPosition && $this->isTooSimpleSetter($propertyName, $setterPosition) &&
232+
$this->isTooSimpleGetter($propertyName, $getterPosition)
233+
) {
234+
$this->getFile()->addWarning(
235+
self::ERROR_GETTER_TOO_SIMPLE,
236+
$getterPosition,
237+
static::CODE_SHOULD_USE_PROPERTY,
238+
$propertyName,
239+
);
240+
241+
$this->getFile()->addWarning(
242+
self::ERROR_SETTER_TOO_SIMPLE,
243+
$setterPosition,
244+
static::CODE_SHOULD_USE_PROPERTY,
245+
$propertyName,
246+
);
247+
}
248+
}
249+
250+
/**
251+
* Registers on the prefixes of a possible property, because is not so easy anymore since type hinting.
252+
*
253+
* @return array|mixed[]
254+
*/
255+
public function register()
256+
{
257+
return [
258+
T_VAR,
259+
T_PUBLIC,
260+
T_PROTECTED,
261+
T_PRIVATE,
262+
];
263+
}
264+
}

‎tests/Sniffs/Functions/Fixtures/NoSimplePropertyMethodSniff/correct/Class.php‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
<?php
2+
3+
namespace BestIt\Sniffs\Functions\NoSimplePropertyMethodSniff;
4+
5+
class CorrectClass
6+
{
7+
/**
8+
* @phpcsSuppress BestIt.Functions.NoSimplePropertyMethod.ShouldUseTypedPropertyDirectly
9+
* @var string
10+
*/
11+
public string $ignore = 'baz';
12+
13+
public $with = 'foo';
14+
15+
public string $without = 'bar';
16+
17+
public function getIgnore(): void
18+
{
19+
return $this->ignore;
20+
}
21+
22+
public function setIgnore(string $ignore): void
23+
{
24+
$this->ignore = $ignore;
25+
}
26+
27+
public function getWith(): string
28+
{
29+
return $this->with;
30+
}
31+
32+
public function setWith(string $with): self
33+
{
34+
$this->with = $with;
35+
36+
return $this;
37+
}
38+
39+
public function setWithout(string $with): self
40+
{
41+
$this->with = $with;
42+
43+
return $this;
44+
}
45+
}

‎tests/Sniffs/Functions/Fixtures/NoSimplePropertyMethodSniff/with_warnings/ShouldUseTypedPropertyDirectly(fluent).9,14.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<?php
2+
3+
namespace BestIt\Sniffs\Functions\NoSimplePropertyMethodSniff;
4+
5+
class ShouldUseTypedPropertyDirectlyInsteadSetter
6+
{
7+
public string $without = 'bar';
8+
9+
public function getWithout(): string
10+
{
11+
return $this->without;
12+
}
13+
14+
public function setWithout(string $with): self
15+
{
16+
$this->without = $with;
17+
18+
return $this;
19+
}
20+
}

‎tests/Sniffs/Functions/Fixtures/NoSimplePropertyMethodSniff/with_warnings/ShouldUseTypedPropertyDirectly(void).9,14.php‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
<?php
2+
3+
namespace BestIt\Sniffs\Functions\NoSimplePropertyMethodSniff;
4+
5+
class ShouldUseTypedPropertyDirectlyInsteadSetter
6+
{
7+
public string $without = 'bar';
8+
9+
public function getWithout(): string
10+
{
11+
return $this->without;
12+
}
13+
14+
public function setWithout(string $with): void
15+
{
16+
$this->without = $with;
17+
}
18+
}

0 commit comments

Comments
 (0)