Feature/deprecations #18

iluuu1994 · 2018-05-06T14:50:37Z

No description provided.

ondrejmirtes · 2018-05-06T16:24:51Z

FYI you can run the build locally and you don't have to wait for Travis 😊 I'd like if you squashed those "fix" commits into the places where the error got created in the first places, for a nicer Git history. Thanks!

iluuu1994 · 2018-05-06T16:31:00Z

Haha yeah I usually do that but the phpcs configuration changed since I've made these commits, I'll squash everything once it's working properly.

iluuu1994 · 2018-05-06T16:33:49Z

@ondrejmirtes Any idea why some of the tested files are actually run? I've got some echo statements that are actually being printed out? Should I just avoid using echo or is this some other issue?

To explain why, I'm testing deprecated property accesses in some tests and the easiest way to use them was just an echo. I could also just store them in a variable.

iluuu1994 · 2018-05-06T17:27:26Z

@ondrejmirtes Ok, I think this is ready. Let me know if there's anything I need to adjust. 🙂

As we discussed before, this PR does currently not support warnings when using deprecated classes or interfaces in type hints.

iluuu1994 · 2018-05-29T08:01:03Z

Any news?

ondrejmirtes · 2018-05-29T08:32:36Z

Don't worry, I'll get to it soon, it will be part of 0.10 😊

iluuu1994 · 2018-05-29T08:35:41Z

Nice 👍

Sorry, patience is not one of my virtues 😄

ondrejmirtes

Also, please add a one-line bullet point to the README 😊 Thanks!

ondrejmirtes · 2018-06-20T18:56:55Z

src/Rules/Deprecations/CallToDeprecatedMethodRule.php

+		}
+
+		$methodCalledOnType = $scope->getType($node->var);
+		$referencedClasses = $methodCalledOnType->getReferencedClasses();


This should rather use TypeUtils::getDirectClassNames.

ondrejmirtes · 2018-06-20T18:58:15Z

src/Rules/Deprecations/AccessDeprecatedPropertyRule.php

+		}
+
+		$propertyAccessedOnType = $scope->getType($node->var);
+		$referencedClasses = $propertyAccessedOnType->getReferencedClasses();


This should rather use TypeUtils::getDirectClassNames().

ondrejmirtes · 2018-06-20T18:59:46Z

src/Rules/Deprecations/AccessDeprecatedStaticPropertyRule.php

+		if ($node->class instanceof Name) {
+			$referredClasses[] = (string) $node->class;
+		} else {
+			$classTypeResult = $this->ruleLevelHelper->findTypeToCheck(


There's a missing:

if ($classTypeResult->getType() instanceof ErrorType) { return []; }

Also, the same method could be used for static methods (not skipping $node->name that is not Name).

ondrejmirtes · 2018-06-20T18:59:53Z

src/Rules/Deprecations/AccessDeprecatedStaticPropertyRule.php

+			$classTypeResult = $this->ruleLevelHelper->findTypeToCheck(
+				$scope,
+				$node->class,
+				'%s' // We don't care about the error message


Empty string please.

ondrejmirtes · 2018-06-20T19:00:13Z

src/Rules/Deprecations/AccessDeprecatedStaticPropertyRule.php

+
+			if ($property instanceof DeprecatableReflection && $property->isDeprecated()) {
+				return [sprintf(
+					'Access to deprecated static property %s of class %s.',


property $%s

ondrejmirtes · 2018-06-20T19:02:02Z

src/Rules/Deprecations/AccessDeprecatedStaticPropertyRule.php

+				'%s' // We don't care about the error message
+			);
+
+			$referredClasses = $classTypeResult->getReferencedClasses();


$referencedClasses

ondrejmirtes · 2018-06-20T19:05:56Z

src/Rules/Deprecations/FetchingClassConstOfDeprecatedClassRule.php

+	 */
+	public function processNode(Node $node, Scope $scope): array
+	{
+		if (!$node->class instanceof Name) {


Employ RuleLevelHelper::findTypeToCheck instead of skipping

ondrejmirtes · 2018-06-20T19:06:10Z

src/Rules/Deprecations/FetchingClassConstOfDeprecatedClassRule.php

+		}
+
+		return [sprintf(
+			'Fetching class constant of deprecated class %s.',


Which class constant?

There's only one class constant 🙂

E.g. stdClass::class

IMHO this rule works for all accessed class constants...

Haha yeah sorry. Apparently I'm confused. But that made me notice that we're totally missing deprecations of constants! I'll fix that too.

ondrejmirtes · 2018-06-20T19:07:53Z

src/Rules/Deprecations/DeprecatedScopeHelper.php

+	public static function isScopeDeprecated(Scope $scope): bool
+	{
+		$class = $scope->getClassReflection();
+		if ($class !== null && $class->isDeprecated()) {


We might also be inside a deprecated trait.

ondrejmirtes · 2018-06-20T19:08:44Z

src/Analyzer/DeprecatedScopeHelper.php

@@ -1,6 +1,6 @@
 <?php declare(strict_types = 1);

-namespace PHPStan\Rules\Deprecations;
+namespace PHPStan\Analyzer;


Noo, leave it in the original place 😊

ondrejmirtes · 2018-06-20T19:09:52Z

I found a few things during the review. Hopefully you will be able to go through them this week, I plan to release 0.10 the next week 😊 Thanks!

ondrejmirtes · 2018-06-20T19:19:58Z

src/Rules/Deprecations/ImplementationOfDeprecatedInterfaceRule.php

+				if ($interface->isDeprecated()) {
+					if (!$class->getNativeReflection()->isAnonymous()) {
+						$errors[] = sprintf(
+							'Implementation of deprecated interface %s in class %s.',


What about Class %s implements deprecated interface %s.?

ondrejmirtes · 2018-06-20T19:20:22Z

src/Rules/Deprecations/InheritanceOfDeprecatedClassRule.php

+			if ($parentClass->isDeprecated()) {
+				if (!$class->getNativeReflection()->isAnonymous()) {
+					$errors[] = sprintf(
+						'Inheritance of deprecated class %s in class %s.',


What about Class %s extends deprecated class %s.?

Also, what about interfaces extending other deprecated interfaces? 😊

Also, what about interfaces extending other deprecated interfaces? 😊

That should be handled in the ImplementationOfDeprecatedInterfaceRule. But tests are missing! Haven't thought about that.

I don't think so, you need a rule that processes Interface_::class for that.

You're right 🙂
I'll make sure it works!

iluuu1994 · 2018-06-20T19:35:41Z

@ondrejmirtes Sure! I'll fix the issues by this weekend then!

ondrejmirtes · 2018-06-20T19:37:44Z

Cool, looking forward to that 😊

iluuu1994 · 2018-06-23T12:07:29Z

@ondrejmirtes

Is there a way in PHPStan to resolve a class name in a certain scope? Something that supports self, parent, static and so forth? I've implemented the check to parent myself here but it's lacking static and self. Obviously I could extend the function but if there's a built in way I'd rather use that.

Broker::getClass unfortunately doesn't accept a scope.

ondrejmirtes · 2018-06-23T12:16:44Z

@iluuu1994 Try Scope::resolveName 😊

iluuu1994 · 2018-06-23T12:28:55Z

@ondrejmirtes Knew it was somewhere 😆

Thanks!

iluuu1994 · 2018-06-23T15:58:05Z

@ondrejmirtes That's it!

I'm still unsure if the implementation of the RuleLevelHelper::findTypeToCheck $unionTypeCriteriaCallback is correct in the InstantiationOfDeprecatedClassRule. Please let me know about that.

Other than that, it's ready for another review.

Majkl578 · 2018-06-23T17:21:03Z

Thinking more and more about this, I feel it doesn't really belong to strict-rules.
Using deprecated methods has nothing to do with strictness, and often there is no way around. As an example, consider ORM 2.x vs. ORM 3.0. You can't often simply abandon deprecated code.

ondrejmirtes · 2018-06-23T18:04:26Z

Yeah I'm also thinking that this might be better as a separate package, it's also huge amount of code 😊 I really wouldn't want to all of this to go to waste, @iluuu1994 what would you think if I created something like phpstan/phpstan-detect-deprecated and make you an owner? 😊

iluuu1994 · 2018-06-23T20:02:14Z

@Majkl578 I don't know, deprecating code does signalize that you should get rid of the dependency sooner or later. Also, both Phan and Psalm have deprecation warnings out of the box.

@ondrejmirtes It probably wouldn't get used quite as much if it requires you to add it manually.

But hey, it's your project 🤷‍♂️ Whatever you see fit is fine.

ondrejmirtes · 2018-06-23T20:11:58Z

On the other hand, you can always ignore it using ignoreErrors 😊 I'll think about it tomorrow and review the new changes 😊

iluuu1994 · 2018-06-23T21:01:54Z

I mean, again, whatever you think is best is fine. If someone is consciously looking for the extension they are gonna find it. We could also mention it on the main repo so that it's easier to discover.

Majkl578 · 2018-06-23T21:41:49Z

deprecating code does signalize that you should get rid of the dependency sooner or later.

As said earlier, not all deprecations are fixable or avoidable.

It probably wouldn't get used quite as much if it requires you to add it manually.

This is kinda selfish, heh. It would be used by people who are interested, not forcibly be people who are not. (And trust me, there is a LOT of such consumers, like Symfony bundles that are supporting SF 2.x + 3.x + 4.x simultaneously).

If someone is consciously looking for the extension they are gonna find it. We could also mention it on the main repo so that it's easier to discover.

This is the correct assumption.

iluuu1994 · 2018-06-23T21:51:52Z

This is kinda selfish, heh. It would be used by people who are interested, not forcibly be people who are not.

That's hardly what I meant. I was my assuming the new deprecation rules add value to the majority of users of this repository (which might be wrong, we can argue about that). By your logic every new rule has to be opt-in because you didn't specifically enable it.

Majkl578 · 2018-06-23T21:59:49Z

Well, even then, I can only repeat myself:

I feel it doesn't really belong to strict-rules.
Using deprecated methods has nothing to do with strictness

By your logic every new rule has to be opt-in because you didn't specifically enable it.

Absolutely not, and please don't question my logic, I never said anthing like that.

ondrejmirtes · 2018-06-24T08:45:23Z

@iluuu1994 This is really awesome body of work! 😊 I'm gonna create an extra package for it and feature it in phpstan/phpstan release notes, README and also in the article, so don't worry, people will know about it 😊

ondrejmirtes · 2018-06-24T08:48:14Z

@iluuu1994 BTW one more thing that should be changed but we can do that in the new repo: When returning error messages, you're using the original class/function/method names as mentioned in the code. You should use ->getName() reported by reflection in order to:

Have correct character case - foo vs. fOO
Account for class aliases (report the original name instead of the use alias)
In case of constants, methods and properties, use declaring class name because the element could have been accessed on a child class or something...

iluuu1994 · 2018-06-24T10:02:01Z

@ondrejmirtes Sure! Let me know when you created the repository so I can migrate the code.

ondrejmirtes · 2018-06-24T11:41:07Z

@iluuu1994 I've sent you an invite to the repo https://github.com/phpstan/phpstan-deprecation-rules 😊 There's a basic skeleton ready. Let me know once the code is there so I can set up Travis CI.

iluuu1994 · 2018-06-24T15:45:12Z

@ondrejmirtes The code is ready, you can set up Travis CI 😊

In the meantime I'll adjust the rules to use the reflection name for reporting errors.

ondrejmirtes · 2018-06-24T16:37:31Z

I fixed the problem with isAnonymous: phpstan/phpstan-deprecation-rules@2ac49f5 Feel free to fix the tests if you don't want to report anything in the deprecated scope.

iluuu1994 · 2018-06-24T16:52:32Z

Awesome! The rule now checks for a deprecated scope as well. Didn't think of anonymous classes when writing that rule.

ondrejmirtes · 2018-06-24T17:09:13Z

OK, closing the PR here, we can continue the discussion over at the new repo 😊 BTW I'm gonna tag 0.10 there some time tonight, so don't be surprised 😊

iluuu1994 · 2018-06-24T17:34:45Z

Sure, thanks man!

Majkl578 · 2018-06-25T00:12:04Z

Already added this to Doctrine Migrations and it looks great! 🎉

See doctrine/migrations#739.

iluuu1994 · 2018-06-25T07:08:11Z

@Majkl578 Nice! Glad to hear!

iluuu1994 force-pushed the feature/deprecations branch from b88079b to 7bf2992 Compare May 6, 2018 16:37

ondrejmirtes requested changes Jun 20, 2018

View reviewed changes

ondrejmirtes reviewed Jun 20, 2018

View reviewed changes

iluuu1994 added 16 commits June 22, 2018 16:22

Add rules for checking deprecated functions and methods

9c20c6a

Implement rule to check deprecated properties

751c048

Implement warning when implementing a deprecated interface

96f3edb

Implement warnings when accessing deprecated static properties

ae9dbc8

Also test traits in AccessDeprecatedStaticPropertyRule

4809b5b

Add tests for deprecated properties from traits

6d4c7b1

Add tests for deprecated methods from traits

6fab32e

Fix spacing in deprecation tests

a2ef842

Add rule to notify when inheriting from a deprecated class

e104c5b

Add rules to warn when instantiating deprecated classes

c140a25

Fix formatting and naming

2da52d1

Add rule to notify when using a deprecated trait

2ca155a

Add support for parent static method calls in deprecation check

7c4da9b

Add check for false positive in static parent method call

31a8dc2

Fix possible null pointer in CallToDeprecatedStaticMethodRule

40a2227

Add test to notify when fetching class constant of deprecated class

49f3352

Use RuleLevelHelper::findTypeToCheck when symbol is not a Name

7a8f54b

iluuu1994 force-pushed the feature/deprecations branch from 920f7f4 to 7a8f54b Compare June 23, 2018 13:57

iluuu1994 added 3 commits June 23, 2018 16:04

Extend DeprecatedScopeHelper to check traits

028e681

Add rule to check for inheritance of deprecated interfaces in interfaces

0e86531

Add deprecation feature to README.md

6cd26a2

ondrejmirtes approved these changes Jun 24, 2018

View reviewed changes

ondrejmirtes closed this Jun 24, 2018

Feature/deprecations #18

Feature/deprecations #18

Conversation

iluuu1994 commented May 6, 2018 • edited Loading

ondrejmirtes commented May 6, 2018

iluuu1994 commented May 6, 2018

iluuu1994 commented May 6, 2018

iluuu1994 commented May 6, 2018

iluuu1994 commented May 29, 2018

ondrejmirtes commented May 29, 2018

iluuu1994 commented May 29, 2018

ondrejmirtes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Jun 20, 2018

ondrejmirtes Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iluuu1994 commented Jun 20, 2018

ondrejmirtes commented Jun 20, 2018

iluuu1994 commented Jun 23, 2018

ondrejmirtes commented Jun 23, 2018

iluuu1994 commented Jun 23, 2018

iluuu1994 commented Jun 23, 2018

Majkl578 commented Jun 23, 2018 • edited Loading

ondrejmirtes commented Jun 23, 2018

iluuu1994 commented Jun 23, 2018

ondrejmirtes commented Jun 23, 2018

iluuu1994 commented Jun 23, 2018

Majkl578 commented Jun 23, 2018 • edited Loading

iluuu1994 commented Jun 23, 2018

Majkl578 commented Jun 23, 2018

ondrejmirtes commented Jun 24, 2018

ondrejmirtes commented Jun 24, 2018

iluuu1994 commented Jun 24, 2018

ondrejmirtes commented Jun 24, 2018

iluuu1994 commented Jun 24, 2018

ondrejmirtes commented Jun 24, 2018

iluuu1994 commented Jun 24, 2018

ondrejmirtes commented Jun 24, 2018

iluuu1994 commented Jun 24, 2018

Majkl578 commented Jun 25, 2018

iluuu1994 commented Jun 25, 2018

iluuu1994 commented May 6, 2018 •

edited

Loading

ondrejmirtes Jun 20, 2018 •

edited

Loading

Majkl578 commented Jun 23, 2018 •

edited

Loading

Majkl578 commented Jun 23, 2018 •

edited

Loading