Skip to content

Commit c51592c

Browse files
nicolas-grekasnicolas-grekas
committed
feature symfony#28329 [Debug] Trigger a deprecation for new parameters not defined in sub classes (GuilhemN)
This PR was merged into the 4.2-dev branch. Discussion ---------- [Debug] Trigger a deprecation for new parameters not defined in sub classes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#28316 | License | MIT | Doc PR | - I'm not sure the way symfony#28316 is implemented is the best so here is an alternative. Instead of counting on a call from the child method, it uses the `DebugClassLoader` and `@param` annotations. If a `@param` annotation is used on a parent but is then not actually implemented in the child class, a deprecation will be thrown. Example: ```php class ClassWithAnnotatedParameters { /** * @param string $foo This is a foo parameter. */ public function fooMethod(string $foo) { } /** * @param string $bar parameter not implemented yet */ public function barMethod(/** string $bar = null */) { } /** * @param Quz $quz parameter not implemented yet */ public function quzMethod(/** Quz $quz = null */) { } } ``` ```php class SubClassWithAnnotatedParameters extends ClassWithAnnotatedParameters { public function fooMethod(string $foo) { } public function barMethod($bar = null) { } public function quzMethod() { } } ``` A deprecation will be triggered because ``SubClassWithAnnotatedParameters::quzMethod()`` which doesn't definee `$quz`. Commits ------- 1f5d8b6 [Debug] Trigger a deprecation for new parameters not defined in sub classes
2 parents 1fc66ff + 1f5d8b6 commit c51592c

File tree

8 files changed

+174
-14
lines changed

8 files changed

+174
-14
lines changed

‎src/Symfony/Bridge/Monolog/Logger.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ class Logger extends BaseLogger implements DebugLoggerInterface, ResetInterface
2323
{
2424
/**
2525
* {@inheritdoc}
26+
*
27+
* @param Request|null $request
2628
*/
2729
public function getLogs(/* Request $request = null */)
2830
{
@@ -39,6 +41,8 @@ public function getLogs(/* Request $request = null */)
3941

4042
/**
4143
* {@inheritdoc}
44+
*
45+
* @param Request|null $request
4246
*/
4347
public function countErrors(/* Request $request = null */)
4448
{

‎src/Symfony/Bridge/Monolog/Processor/DebugProcessor.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,8 @@ public function __invoke(array $record)
5858

5959
/**
6060
* {@inheritdoc}
61+
*
62+
* @param Request|null $request
6163
*/
6264
public function getLogs(/* Request $request = null */)
6365
{
@@ -78,6 +80,8 @@ public function getLogs(/* Request $request = null */)
7880

7981
/**
8082
* {@inheritdoc}
83+
*
84+
* @param Request|null $request
8185
*/
8286
public function countErrors(/* Request $request = null */)
8387
{

‎src/Symfony/Component/Debug/DebugClassLoader.php

Lines changed: 63 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111

1212
namespace Symfony\Component\Debug;
1313

14+
use PHPUnit\Framework\MockObject\Matcher\StatelessInvocation;
15+
1416
/**
1517
* Autoloader checking if the class is really defined in the file found.
1618
*
@@ -21,6 +23,7 @@
2123
* @author Fabien Potencier <fabien@symfony.com>
2224
* @author Christophe Coevoet <stof@notk.org>
2325
* @author Nicolas Grekas <p@tchwork.com>
26+
* @author Guilhem Niot <guilhem.niot@gmail.com>
2427
*/
2528
class DebugClassLoader
2629
{
@@ -34,6 +37,7 @@ class DebugClassLoader
3437
private static $deprecated = array();
3538
private static $internal = array();
3639
private static $internalMethods = array();
40+
private static $annotatedParameters = array();
3741
private static $darwinCache = array('/' => array('/', array()));
3842

3943
public function __construct(callable $classLoader)
@@ -200,9 +204,12 @@ private function checkClass($class, $file = null)
200204
}
201205
}
202206

207+
$parent = \get_parent_class($class);
203208
$parentAndTraits = \class_uses($name, false);
204-
if ($parent = \get_parent_class($class)) {
205-
$parentAndTraits[] = $parent;
209+
$parentAndOwnInterfaces = $this->getOwnInterfaces($name, $parent);
210+
if ($parent) {
211+
$parentAndTraits[$parent] = $parent;
212+
$parentAndOwnInterfaces[$parent] = $parent;
206213

207214
if (!isset(self::$checkedClasses[$parent])) {
208215
$this->checkClass($parent);
@@ -214,7 +221,7 @@ private function checkClass($class, $file = null)
214221
}
215222

216223
// Detect if the parent is annotated
217-
foreach ($parentAndTraits + $this->getOwnInterfaces($name, $parent) as $use) {
224+
foreach ($parentAndTraits + $parentAndOwnInterfaces as $use) {
218225
if (!isset(self::$checkedClasses[$use])) {
219226
$this->checkClass($use);
220227
}
@@ -229,11 +236,17 @@ private function checkClass($class, $file = null)
229236
}
230237
}
231238

232-
// Inherit @final and @internal annotations for methods
239+
// Inherit @final, @internal and @param annotations for methods
233240
self::$finalMethods[$name] = array();
234241
self::$internalMethods[$name] = array();
235-
foreach ($parentAndTraits as $use) {
236-
foreach (array('finalMethods', 'internalMethods') as $property) {
242+
self::$annotatedParameters[$name] = array();
243+
$map = array(
244+
'finalMethods' => $parentAndTraits,
245+
'internalMethods' => $parentAndTraits,
246+
'annotatedParameters' => $parentAndOwnInterfaces, // We don't parse traits params
247+
);
248+
foreach ($map as $property => $uses) {
249+
foreach ($uses as $use) {
237250
if (isset(self::${$property}[$use])) {
238251
self::${$property}[$name] = self::${$property}[$name] ? self::${$property}[$use] + self::${$property}[$name] : self::${$property}[$use];
239252
}
@@ -258,20 +271,56 @@ private function checkClass($class, $file = null)
258271
}
259272
}
260273

261-
// Method from a trait
262-
if ($method->getFilename() !== $refl->getFileName()) {
274+
// To read method annotations
275+
$doc = $method->getDocComment();
276+
277+
if (isset(self::$annotatedParameters[$name][$method->name])) {
278+
$definedParameters = array();
279+
foreach ($method->getParameters() as $parameter) {
280+
$definedParameters[$parameter->name] = true;
281+
}
282+
283+
foreach (self::$annotatedParameters[$name][$method->name] as $parameterName => $deprecation) {
284+
if (!isset($definedParameters[$parameterName]) && !($doc && preg_match("/\\n\\s+\\* @param (.*?)(?<= )\\\${$parameterName}\\b/", $doc))) {
285+
@trigger_error(sprintf($deprecation, $name), E_USER_DEPRECATED);
286+
}
287+
}
288+
}
289+
290+
if (!$doc) {
263291
continue;
264292
}
265293

266-
// Detect method annotations
267-
if (false === $doc = $method->getDocComment()) {
294+
$finalOrInternal = false;
295+
296+
// Skip methods from traits
297+
if ($method->getFilename() === $refl->getFileName()) {
298+
foreach (array('final', 'internal') as $annotation) {
299+
if (false !== \strpos($doc, $annotation) && preg_match('#\n\s+\* @'.$annotation.'(?:( .+?)\.?)?\r?\n\s+\*(?: @|/$)#s', $doc, $notice)) {
300+
$message = isset($notice[1]) ? preg_replace('#\s*\r?\n \* +#', ' ', $notice[1]) : '';
301+
self::${$annotation.'Methods'}[$name][$method->name] = array($name, $message);
302+
$finalOrInternal = true;
303+
}
304+
}
305+
}
306+
307+
if ($finalOrInternal || $method->isConstructor() || false === \strpos($doc, '@param') || StatelessInvocation::class === $name) {
268308
continue;
269309
}
270310

271-
foreach (array('final', 'internal') as $annotation) {
272-
if (false !== \strpos($doc, $annotation) && preg_match('#\n\s+\* @'.$annotation.'(?:( .+?)\.?)?\r?\n\s+\*(?: @|/$)#s', $doc, $notice)) {
273-
$message = isset($notice[1]) ? preg_replace('#\s*\r?\n \* +#', ' ', $notice[1]) : '';
274-
self::${$annotation.'Methods'}[$name][$method->name] = array($name, $message);
311+
if (!preg_match_all('#\n\s+\* @param (.*?)(?<= )\$([a-zA-Z0-9_\x7f-\xff]++)#', $doc, $matches, PREG_SET_ORDER)) {
312+
continue;
313+
}
314+
if (!isset(self::$annotatedParameters[$name][$method->name])) {
315+
$definedParameters = array();
316+
foreach ($method->getParameters() as $parameter) {
317+
$definedParameters[$parameter->name] = true;
318+
}
319+
}
320+
foreach ($matches as list(, $parameterType, $parameterName)) {
321+
if (!isset($definedParameters[$parameterName])) {
322+
$parameterType = trim($parameterType);
323+
self::$annotatedParameters[$name][$method->name][$parameterName] = sprintf('The "%%s::%s()" method will require a new "%s$%s" argument in the next major version of its parent class "%s", not defining it is deprecated.', $method->name, $parameterType ? $parameterType.' ' : '', $parameterName, $method->class);
275324
}
276325
}
277326
}

‎src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,24 @@ class_exists('Test\\'.__NAMESPACE__.'\\ExtendsInternals', true);
272272
'The "Symfony\Component\Debug\Tests\Fixtures\InternalTrait2::internalMethod()" method is considered internal. It may change without further notice. You should not extend it from "Test\Symfony\Component\Debug\Tests\ExtendsInternals".',
273273
));
274274
}
275+
276+
public function testExtendedMethodDefinesNewParameters()
277+
{
278+
$deprecations = array();
279+
set_error_handler(function ($type, $msg) use (&$deprecations) { $deprecations[] = $msg; });
280+
$e = error_reporting(E_USER_DEPRECATED);
281+
282+
class_exists(__NAMESPACE__.'\\Fixtures\SubClassWithAnnotatedParameters', true);
283+
284+
error_reporting($e);
285+
restore_error_handler();
286+
287+
$this->assertSame(array(
288+
'The "Symfony\Component\Debug\Tests\Fixtures\SubClassWithAnnotatedParameters::quzMethod()" method will require a new "Quz $quz" argument in the next major version of its parent class "Symfony\Component\Debug\Tests\Fixtures\ClassWithAnnotatedParameters", not defining it is deprecated.',
289+
'The "Symfony\Component\Debug\Tests\Fixtures\SubClassWithAnnotatedParameters::whereAmI()" method will require a new "bool $matrix" argument in the next major version of its parent class "Symfony\Component\Debug\Tests\Fixtures\InterfaceWithAnnotatedParameters", not defining it is deprecated.',
290+
'The "Symfony\Component\Debug\Tests\Fixtures\SubClassWithAnnotatedParameters::isSymfony()" method will require a new "true $yes" argument in the next major version of its parent class "Symfony\Component\Debug\Tests\Fixtures\ClassWithAnnotatedParameters", not defining it is deprecated.',
291+
), $deprecations);
292+
}
275293
}
276294

277295
class ClassLoader

‎src/Symfony/Component/Debug/Tests/Fixtures/ClassWithAnnotatedParameters.php

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
<?php
2+
3+
namespace Symfony\Component\Debug\Tests\Fixtures;
4+
5+
class ClassWithAnnotatedParameters
6+
{
7+
/**
8+
* @param string $foo this is a foo parameter
9+
*/
10+
public function fooMethod(string $foo)
11+
{
12+
}
13+
14+
/**
15+
* @param string $bar parameter not implemented yet
16+
*/
17+
public function barMethod(/* string $bar = null */)
18+
{
19+
}
20+
21+
/**
22+
* @param Quz $quz parameter not implemented yet
23+
*/
24+
public function quzMethod(/* Quz $quz = null */)
25+
{
26+
}
27+
28+
/**
29+
* @param true $yes
30+
*/
31+
public function isSymfony()
32+
{
33+
}
34+
}

‎src/Symfony/Component/Debug/Tests/Fixtures/InterfaceWithAnnotatedParameters.php

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<?php
2+
3+
namespace Symfony\Component\Debug\Tests\Fixtures;
4+
5+
/**
6+
* Ensures a deprecation is triggered when a new parameter is not declared in child classes.
7+
*/
8+
interface InterfaceWithAnnotatedParameters
9+
{
10+
/**
11+
* @param bool $matrix
12+
*/
13+
public function whereAmI();
14+
}

‎src/Symfony/Component/Debug/Tests/Fixtures/SubClassWithAnnotatedParameters.php

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<?php
2+
3+
namespace Symfony\Component\Debug\Tests\Fixtures;
4+
5+
class SubClassWithAnnotatedParameters extends ClassWithAnnotatedParameters implements InterfaceWithAnnotatedParameters
6+
{
7+
use TraitWithAnnotatedParameters;
8+
9+
public function fooMethod(string $foo)
10+
{
11+
}
12+
13+
public function barMethod($bar = null)
14+
{
15+
}
16+
17+
public function quzMethod()
18+
{
19+
}
20+
21+
public function whereAmI()
22+
{
23+
}
24+
}

‎src/Symfony/Component/Debug/Tests/Fixtures/TraitWithAnnotatedParameters.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php
2+
3+
namespace Symfony\Component\Debug\Tests\Fixtures;
4+
5+
trait TraitWithAnnotatedParameters
6+
{
7+
/**
8+
* `@param` annotations in traits are not parsed.
9+
*/
10+
public function isSymfony()
11+
{
12+
}
13+
}

0 commit comments

Comments
 (0)