8000 [Debug] Trigger a deprecation for new parameters not defined in sub classes by GuilhemN · Pull Request #28329 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Debug] Trigger a deprecation for new parameters not defined in sub classes #28329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 21, 2018
Merged

[Debug] Trigger a deprecation for new parameters not defined in sub classes #28329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Symfony/Bridge/Monolog/Logger.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ class Logger extends BaseLogger implements DebugLoggerInterface, ResetInterface
{
/**
* {@inheritdoc}
*
* @param Request|null $request
*/
public function getLogs(/* Request $request = null */)
{
Expand All @@ -39,6 +41,8 @@ public function getLogs(/* Request $request = null */)

/**
* {@inheritdoc}
*
* @param Request|null $request
*/
public function countErrors(/* Request $request = null */)
{
Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Bridge/Monolog/Processor/DebugProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ public function __invoke(array $record)

/**
* {@inheritdoc}
*
* @param Request|null $request
*/
public function getLogs(/* Request $request = null */)
{
Expand All @@ -78,6 +80,8 @@ public function getLogs(/* Request $request = null */)

/**
* {@inheritdoc}
*
* @param Request|null $request
*/
public function countErrors(/* Request $request = null */)
{
Expand Down
77 changes: 63 additions & 14 deletions src/Symfony/Component/Debug/DebugClassLoader.php
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

namespace Symfony\Component\Debug;

use PHPUnit\Framework\MockObject\Matcher\StatelessInvocation;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related to sebastianbergmann/phpunit-mock-objects#427


/**
* Autoloader checking if the class is really defined in the file found.
*
Expand All @@ -21,6 +23,7 @@
* @author Fabien Potencier <fabien@symfony.com>
* @author Christophe Coevoet <stof@notk.org>
* @author Nicolas Grekas <p@tchwork.com>
* @author Guilhem Niot <guilhem.niot@gmail.com>
*/
class DebugClassLoader
{
Expand All @@ -34,6 +37,7 @@ class DebugClassLoader
private static $deprecated = array();
private static $internal = array();
private static $internalMethods = array();
private static $annotatedParameters = array();
private static $darwinCache = array('/' => array('/', array()));

public function __construct(callable $classLoader)
Expand Down Expand Up @@ -200,9 +204,12 @@ private function checkClass($class, $file = null)
}
}

$parent = \get_parent_class($class);
$parentAndTraits = \class_uses($name, false);
if ($parent = \get_parent_class($class)) {
$parentAndTraits[] = $parent;
$parentAndOwnInterfaces = $this->getOwnInterfaces($name, $parent);
if ($parent) {
$parentAndTraits[$parent] = $parent;
$parentAndOwnInterfaces[$parent] = $parent;

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

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

// Inherit @final and @internal annotations for methods
// Inherit @final, @internal and @param annotations for methods
self::$finalMethods[$name] = array();
self::$internalMethods[$name] = array();
foreach ($parentAndTraits as $use) {
foreach (array('finalMethods', 'internalMethods') as $property) {
self::$annotatedParameters[$name] = array();
$map = array(
'finalMethods' => $parentAndTraits,
'internalMethods' => $parentAndTraits,
'annotatedParameters' => $parentAndOwnInterfaces, // We don't parse traits params
);
foreach ($map as $property => $uses) {
foreach ($uses as $use) {
if (isset(self::${$property}[$use])) {
self::${$property}[$name] = self::${$property}[$name] ? self::${$property}[$use] + self::${$property}[$name] : self::${$property}[$use];
}
Expand All @@ -258,20 +271,56 @@ private function checkClass($class, $file = null)
}
}

// Method from a trait
if ($method->getFilename() !== $refl->getFileName()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be kept imo, I think we also don't want methods from traits being checked against their own @param annotations.

Copy link
Member
@nicolas-grekas nicolas-grekas Sep 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth a new test case :)
we also want to report implementations provided by traits that don't provide the argument declared by parents
and also to report missing arguments declared by @param on traits

Copy link
Contributor Author
@GuilhemN GuilhemN Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also want to report implementations provided by traits that don't provide the argument declared by parents

👍, you were right about this change.

and also to report missing arguments declared by @param on traits

Thinking of it again, I prefer not parsing @param in traits directly since PHP allows a full overwriting of their methods, instead their methods are checked as if they belong to the sub classes using them.

// To read method annotations
$doc = $method->getDocComment();

if (isset(self::$annotatedParameters[$name][$method->name])) {
$definedParameters = array();
foreach ($method->getParameters() as $parameter) {
$definedParameters[$parameter->name] = true;
}

foreach (self::$annotatedParameters[$name][$method->name] as $parameterName => $deprecation) {
if (!isset($definedParameters[$parameterName]) && !($doc && preg_match("/\\n\\s+\\* @param (.*?)(?<= )\\\${$parameterName}\\b/", $doc))) {
@trigger_error(sprintf($deprecation, $name), E_USER_DEPRECATED);
}
}
}

if (!$doc) {
continue;
}

// Detect method annotations
if (false === $doc = $method->getDocComment()) {
$finalOrInternal = false;

// Skip methods from traits
if ($method->getFilename() === $refl->getFileName()) {
foreach (array('final', 'internal') as $annotation) {
if (false !== \strpos($doc, $annotation) && preg_match('#\n\s+\* @'.$annotation.'(?:( .+?)\.?)?\r?\n\s+\*(?: @|/$)#s', $doc, $notice)) {
$message = isset($notice[1]) ? preg_replace('#\s*\r?\n \* +#', ' ', $notice[1]) : '';
self::${$annotation.'Methods'}[$name][$method->name] = array($name, $message);
$finalOrInternal = true;
}
}
}

if ($finalOrInternal || $method->isConstructor() || false === \strpos($doc, '@param') || StatelessInvocation::class === $name) {
continue;
}

foreach (array('final', 'internal') as $annotation) {
if (false !== \strpos($doc, $annotation) && preg_match('#\n\s+\* @'.$annotation.'(?:( .+?)\.?)?\r?\n\s+\*(?: @|/$)#s', $doc, $notice)) {
$message = isset($notice[1]) ? preg_replace('#\s*\r?\n \* +#', ' ', $notice[1]) : '';
self::${$annotation.'Methods'}[$name][$method->name] = array($name, $message);
if (!preg_match_all('#\n\s+\* @param (.*?)(?<= )\$([a-zA-Z0-9_\x7f-\xff]++)#', $doc, $matches, PREG_SET_ORDER)) {
continue;
}
if (!isset(self::$annotatedParameters[$name][$method->name])) {
$definedParameters = array();
foreach ($method->getParameters() as $parameter) {
$definedParameters[$parameter->name] = true;
}
}
foreach ($matches as list(, $parameterType, $parameterName)) {
if (!isset($definedParameters[$parameterName])) {
$parameterType = trim($parameterType);
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);
}
}
}
Expand Down
18 changes: 18 additions & 0 deletions src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,24 @@ class_exists('Test\\'.__NAMESPACE__.'\\ExtendsInternals', true);
'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".',
));
}

public function testExtendedMethodDefinesNewParameters()
{
$deprecations = array();
set_error_handler(function ($type, $msg) use (&$deprecations) { $deprecations[] = $msg; });
$e = error_reporting(E_USER_DEPRECATED);

class_exists(__NAMESPACE__.'\\Fixtures\SubClassWithAnnotatedParameters', true);

error_reporting($e);
restore_error_handler();

$this->assertSame(array(
'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.',
'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.',
'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.',
), $deprecations);
}
}

class ClassLoader
Expand Down
34 changes: 34 additions & 0 deletions src/Symfony/Component/Debug/Tests/Fixtures/ClassWithAnnotatedParameters.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

namespace Symfony\Component\Debug\Tests\Fixtures;

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 */)
{
}

/**
* @param true $yes
*/
public function isSymfony()
{
}
}
14 changes: 14 additions & 0 deletions src/Symfony/Component/Debug/Tests/Fixtures/InterfaceWithAnnotatedParameters.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php

namespace Symfony\Component\Debug\Tests\Fixtures;

/**
* Ensures a deprecation is triggered when a new parameter is not declared in child classes.
*/
interface InterfaceWithAnnotatedParameters
{
/**
* @param bool $matrix
*/
public function whereAmI();
}
24 changes: 24 additions & 0 deletions src/Symfony/Component/Debug/Tests/Fixtures/SubClassWithAnnotatedParameters.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

namespace Symfony\Component\Debug\Tests\Fixtures;

class SubClassWithAnnotatedParameters extends ClassWithAnnotatedParameters implements InterfaceWithAnnotatedParameters
{
use TraitWithAnnotatedParameters;

public function fooMethod(string $foo)
{
}

public function barMethod($bar = null)
{
}

public function quzMethod()
{
}

public function whereAmI()
{
}
}
13 changes: 13 additions & 0 deletions src/Symfony/Component/Debug/Tests/Fixtures/TraitWithAnnotatedParameters.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

namespace Symfony\Component\Debug\Tests\Fixtures;

trait TraitWithAnnotatedParameters
{
/**
* `@param` annotations in traits are not parsed.
*/
public function isSymfony()
{
}
}
0