Skip to content
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.

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

Support messages on the deprecated PhpDoc tag #1983

Merged
merged 6 commits into from
May 28, 2019

Conversation

mglaman
Copy link
Contributor

@mglaman mglaman commented Mar 12, 2019

Adds support for returning the descriptive messages behind @deprecated tags.

In reference to phpstan/phpstan-deprecation-rules#2

src/PhpDoc/ResolvedPhpDocBlock.php Outdated Show resolved Hide resolved
src/Reflection/ClassReflection.php Outdated Show resolved Hide resolved
@mglaman mglaman force-pushed the deprecation-messages branch 5 times, most recently from 6b325ac to 1b72d7f Compare March 12, 2019 20:20
@mglaman mglaman marked this pull request as ready for review March 12, 2019 20:20
@mglaman
Copy link
Contributor Author

mglaman commented Mar 13, 2019

1) PHPStan\Rules\Comparison\ImpossibleCheckTypeStaticMethodCallRuleTest::testRule
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'13: Call to static method PHPStan\Tests\AssertionClass::assertInt() with int will always evaluate to true.
-14: Call to static method PHPStan\Tests\AssertionClass::assertInt() with string will always evaluate to false.
-31: Call to static method PHPStan\Tests\AssertionClass::assertInt() with int will always evaluate to true.
-32: Call to static method PHPStan\Tests\AssertionClass::assertInt() with string will always evaluate to false.
-33: Call to static method PHPStan\Tests\AssertionClass::assertInt() with 1 and 2 will always evaluate to true.
-34: Call to static method PHPStan\Tests\AssertionClass::assertInt() with arguments 1, 2 and 3 will always evaluate to true.'
+'00: Internal error: Undefined index: deprecatedTag

I think this is an error due to __set_state on a class. Need to review.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Please also bump version cache in TypeNodeResolverExtension. There's also a high probability of conflicts with #1980 so the second PR merged will have to resolve them. /cc @marcospassos

The implementations should look almost identical so please check each other's PRs :) #1980 is missing populating all the reflection classes with the value so that should be added there.

src/Reflection/DeprecatableReflection.php Outdated Show resolved Hide resolved
src/Testing/TestCase.php Outdated Show resolved Hide resolved
@mglaman
Copy link
Contributor Author

mglaman commented Mar 14, 2019

@ondrejmirtes double checking the cache key bump should be in TypeNodeResolver at this method? That is the one spot I see a place to bust cache keys from the interface's getCacheKey method

	public function getCacheKey(): string
	{
		$key = 'v50';
		foreach ($this->extensions as $extension) {
			$key .= sprintf('-%s', $extension->getCacheKey());
		}

		return $key;
	}

@mglaman
Copy link
Contributor Author

mglaman commented Mar 18, 2019

I've fixed the conflict with master and also improved the deprecation messaging. Handling multiline deprecation messages felt kind of hacky. I opened phpstan/phpdoc-parser#23 assuming the Ast update might make this easier. Tests updated as well. This matches real life Symfony and Drupal deprecation scenarios.

@mglaman mglaman force-pushed the deprecation-messages branch 2 times, most recently from 47f2e12 to cb6b6aa Compare April 19, 2019 01:34
@mglaman
Copy link
Contributor Author

mglaman commented Apr 19, 2019

This failure is blowing my mind. It doesn't fail locally (macOS, 7.3) but does on Travis

1) PHPStan\Rules\Comparison\ImpossibleCheckTypeStaticMethodCallRuleTest::testRule
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'13: Call to static method PHPStan\Tests\AssertionClass::assertInt() with int will always evaluate to true.
-14: Call to static method PHPStan\Tests\AssertionClass::assertInt() with string will always evaluate to false.
-31: Call to static method PHPStan\Tests\AssertionClass::assertInt() with int will always evaluate to true.
-32: Call to static method PHPStan\Tests\AssertionClass::assertInt() with string will always evaluate to false.
-33: Call to static method PHPStan\Tests\AssertionClass::assertInt() with 1 and 2 will always evaluate to true.
-34: Call to static method PHPStan\Tests\AssertionClass::assertInt() with arguments 1, 2 and 3 will always evaluate to true.'
+'00: Internal error: Undefined index: deprecatedTag
+Run PHPStan with --debug option and post the stack trace to:
+https://github.com/phpstan/phpstan/issues/new'

The only thing I can find that would cause this is ResolvedPhpDocBlock:

	/**
	 * @param mixed[] $properties
	 * @return self
	 */
	public static function __set_state(array $properties): self
	{
		return new self(
			$properties['varTags'],
			$properties['methodTags'],
			$properties['propertyTags'],
			$properties['paramTags'],
			$properties['returnTag'],
			$properties['throwsTag'],
			$properties['deprecatedTag'],
			$properties['isDeprecated'],
			$properties['isInternal'],
			$properties['isFinal']
		);
	}

@mglaman
Copy link
Contributor Author

mglaman commented Apr 19, 2019

🤦🏼‍♂️The error was due to TravisCI caches.

@ondrejmirtes this should be ready for review again. If you purge the PR caches I can revert 93b7b77, unless you don't mind the double increment.

@goba
Copy link

goba commented Apr 23, 2019

Really excited about this! This will help people resolve deprecations so much!

@ondrejmirtes
Copy link
Member

I added two commits:

fc5ed56 - I hope my intention is clear
ea69061

About the second commit - it seemed very peculiar to me that you introduced some changes to phpdoc-parser that are not used here. So I tried to take advantage of them by releasing new version of phpdoc-parser, but something broke - multiline descriptions are now parsed wrong way. Can you look into it? Thanks :)

@mglaman
Copy link
Contributor Author

mglaman commented Apr 23, 2019

fc5ed56 makes sense, I realized the code became fragmented in my handling. This seems to solidify it.

For the second commit: I wasn't sure if that PR would land, or how to uptake it :) I was going to try integrating it tonight but you beat me! I'll review this tonight.

@mglaman
Copy link
Contributor Author

mglaman commented Apr 24, 2019

This makes more sense after reading it again. The deprecation rules should provide the format of "Foo is deprecated {description"

		if ($this->isDeprecated) {
			if ($this->deprecatedDescription !== null && $this->deprecatedDescription !== '') {
				return $this->getName() . ' is deprecated ' . $this->deprecatedDescription;
			}
			return $this->getName() . ' is deprecated.';
			return $this->deprecatedDescription;
		}

I'm digging in to see why the following case fails. I'm guessing I missed a scenario in the phpdoc-parser, but I will validate that.

/**
 * Class with multiple tags and children.
 *
 * @deprecated in Foo 1.1.0 and will be removed in 1.5.0, use
 *   \Foo\Bar\NotDeprecated instead.
 *
 * @author Foo Baz <foo@baz.com>
 */
class DeprecatedWithMultipleTags

@mglaman
Copy link
Contributor Author

mglaman commented Apr 24, 2019

I'm going to investigate phpdoc-parser and what I missed.
Screen Shot 2019-04-24 at 8 14 56 AM

@mglaman
Copy link
Contributor Author

mglaman commented Apr 24, 2019

Opened phpstan/phpdoc-parser#25, working on a PR.

It's my code in this PR, started jumping around too much.

@@ -212,6 +217,45 @@ private function resolveThrowsTags(PhpDocNode $phpDocNode, NameScope $nameScope)
return new ThrowsTag(TypeCombinator::union(...$types));
}

private function resolveDeprecatedTag(PhpDocNode $phpDocNode, NameScope $nameScope): ?\PHPStan\PhpDoc\Tag\DeprecatedTag
{
foreach ($phpDocNode->getDeprecatedTagValues() as $deprecatedTagValue) {
Copy link
Contributor Author

@mglaman mglaman Apr 24, 2019

Choose a reason for hiding this comment

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

🤔 @ondrejmirtes so this whole block of code "reassembles" multiple line deprecation comments. Should that go here or in the phpdoc-parser library? It didn't occur to me previously. But now that we have the code in phpdoc-parser it popped up in my head.

Copy link
Member

Choose a reason for hiding this comment

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

Try making it part of phpdoc-parser so that the node description field already has the text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will do that

@mglaman mglaman changed the title Support messages on the deprecated PhpDoc tag [Blocked] Support messages on the deprecated PhpDoc tag May 24, 2019
@ondrejmirtes ondrejmirtes changed the title [Blocked] Support messages on the deprecated PhpDoc tag Support messages on the deprecated PhpDoc tag May 28, 2019
@ondrejmirtes
Copy link
Member

Let's see if this builds :)

@ondrejmirtes ondrejmirtes merged commit 77b355f into phpstan:master May 28, 2019
@ondrejmirtes
Copy link
Member

🎉

@mglaman
Copy link
Contributor Author

mglaman commented May 28, 2019

🤩thank you! I will move over to the deprecation rules extension!

@ondrejmirtes
Copy link
Member

Don't worry, I'm already working on it. Want to release it tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants