[shopsys] automated fixes and additions of annotations for extended classes #1344

vitek-rostislav · 2019-08-22T11:07:31Z

Q	A
Description, reason for the PR	When using class inheritance (and interface implementations) for extending the behavior of the `framework` from `project-base`, static analysis tools are not able to understand the extensions, which makes the DX worse and PHPStan inspections fail. We want to use the higher level of PHPStan even in `project-base` and do not force the project developers to do the boring things manually so we created the automated tool that makes the static analysis understand the extended code properly.
New feature	Yes
BC breaks	No
Fixes issues	closes #914
Have you read and signed our License Agreement for contributions?	Yes

TODO

add docs

grossmannmartin

So far reviewed first three commits

...ork/src/DependencyInjection/Compiler/RegisterProjectFrameworkClassExtensionsCompilerPass.php

packages/framework/src/Resources/config/services.yml

packages/framework/src/ShopsysFrameworkBundle.php

packages/framework/src/Resources/config/services/commands.yml

packages/framework/src/Command/FixExtendedClassesAnnotationsCommand.php

grossmannmartin

Hi @vitek-rostislav, I finished the review.

First of all, I'm really thrilled about this new functionality! I really think it helps a lot.

I made some suggestions and comments to the code, please check them out.

packages/framework/src/Command/FixExtendedClassesAnnotationsCommand.php

composer.json

packages/backend-api/install/build.xml.patch

packages/framework/src/Command/FixExtendedClassesAnnotationsCommand.php

packages/framework/src/Resources/config/services/commands.yml

packages/framework/src/Command/FixExtendedClassesAnnotationsCommand.php

docs/introduction/framework-extensibility.md

grossmannmartin

Hi, awesome job with refactoring!

I reviewed the changes excluding the tests and have few things to consider.
In the end, don't forget to add changes generated with this command (but it will be better after the command itself is reviewed and tested – another PR maybe?)

And PHPStan fails on dummy classes for tests, these could be excluded from analysis (maybe renamed like ChildClass3.php.test but I'm not sure if it will work.

I take a look at tests tomorrow

packages/framework/src/Component/ClassExtension/AnnotationsAdder.php

packages/framework/src/Component/ClassExtension/AnnotationsReplacementsMap.php

packages/framework/src/Component/ClassExtension/ClassExtensionRegistry.php

packages/framework/src/Component/ClassExtension/MethodAnnotationsFactory.php

packages/framework/src/Component/ClassExtension/PropertyAnnotationsFactory.php

packages/framework/src/Resources/config/services.yml

grossmannmartin

finished the review. just last thing about phpstan ignored rule

packages/framework/phpstan.neon

phpstan.neon

LukasHeinz

Summary of business validation discussion:

there should be two separate upgrade notes - one for using annotation fix and second for lvl4 PHP stan because these are two separate issues
the annotation-fix should be part of coding standards (or something like this) and should behave the same way (no fixes without confirmation)

grossmannmartin

Hi, I will go to hell for more and more requested changes

project-base/tests/ShopBundle/Functional/Controller/ProductRenameRedirectPreviousUrlTest.php

project-base/src/Shopsys/ShopBundle/DataFixtures/Performance/ProductDataFixture.php

project-base/src/Shopsys/ShopBundle/DataFixtures/Demo/SettingValueDataFixture.php

project-base/src/Shopsys/ShopBundle/DataFixtures/Demo/ProductAccessoriesDataFixture.php

project-base/src/Shopsys/ShopBundle/DataFixtures/Demo/MultidomainSettingValueDataFixture.php

packages/framework/src/Command/FixExtendedClassesAnnotationsCommand.php

docs/upgrade/UPGRADE-v8.1.0-dev.md

docs/introduction/framework-extensibility.md

packages/framework/build.xml

docs/introduction/framework-extensibility.md

grossmannmartin

Reviewed last two commits

packages/framework/src/Component/ClassExtension/MethodAnnotationsFactory.php

…ject classes that extend the classes (or implement interfaces) from framework - the registry will be used for automated annotations fixes and additions in project classes - this will make PHPStan and PHPStorm understand the code of Shopsys Framework based project properly - the extensions information is not collected in production environment as it is not necessary there

- the class is unit tested - AnnotationsReplacementsMap was extracted as well because the mappings are used on multiple places in the command

… replaceInPropertyType() extracted to AnnotationsReplacer - added unit tests for the methods

…r annotations replacements - getRelevantFrameworkAnnotationsPattern() was broken by rewritting the history of the original commit - the functionality extracted to AnnotationsReplacementsMap::getPatternForAny

- AnnotationsReplacementsMap is an autowired DIC service as well - it's constructor's argument $classExtensionMap is injected using expression language - see https://symfony.com/doc/3.4/service_container/expression_language.html

…etter reflection class

@method

…and @method annotation extracted to separate classes - MethodAnnotationsFactory - responsible for providing @method annotation lines, unit tested - PropertyAnnotationsFactory - responsible for providing @Property annotation lines, unit tested - AnnotationsAdder - respnsible for adding the annotations to classes, unit tested - FileContentsReplacer - a simple helper class wrapping file_get_contents, str_replace, and file_put_contents calls sequence

- all the automated fixes and additions of annotations are applied - the standards are failing intentionally on this commit as there are several phpstan rules violations that need to be fixed manually yet

…matically using "php phing fix-annotations" - sometimes, there is an extended entity returned directly by calling a framework service thanks to EntityNameResolver magic that phpstan does not understand - there is a workaround for this - a project developer can create an empty service extending the framework one (e.g. ShopBundle\ProductFacade extends FrameworkBundle\BaseProductFacade) - in such a case the developer can generate annotations for his extended class using "php phing fix-annotations" that will wix the phpstan rules violations as well

grossmannmartin

Reviewed changes from testing.
One thing and you have typo in the commit message

- ClassExtensionRegistry: extended ontrollers are now registered as well
+ ClassExtensionRegistry: extended controllers are now registered as well

packages/framework/src/Component/ClassExtension/ClassExtensionRegistry.php

…rbose output - FixExtendedClassesAnnotationsCommand renamed to ExtendedClassesAnnotationsCommand - filenames that were fixed (or need to be fixed when using "dry-run" mode) are reported

…hing targets "standards", "standards-diff", "standards-fix", and "standards-fix-diff" - there is a new build property "check-and-fix-annotations" that allows project developers to skip the checks and fixes of annotations when they do not want to include them in their "standards-*" targets - done in such a way to avoid possible BC breaks

@method

…into account as well - when a method has a parameter of a type that is extended in project, appropriate @method annotation is created - proper parameter types are included in @method annotations - done to achieve correct IDE autocompletion

- parameter types are now taken into account as well

@var

…the annotations fixer - proper format is "/** @var Type $variableName */" above the variable - the annotations were malformatted before so we fixed what we noticed :)

…nnotations-fix" and "annotations-check" - in accordance with our standards - https://github.com/shopsys/shopsys/blob/master/docs/contributing/guidelines-for-phing-targets.md#naming-conventions

…tions-*" targets moved to "Coding Standards section"

All the objections were addressed, approved personally 😁

simara-svatopluk · 2019-09-06T06:30:01Z

🚀

grossmannmartin requested changes Aug 22, 2019

View reviewed changes

grossmannmartin requested changes Aug 23, 2019

View reviewed changes

vitek-rostislav force-pushed the rv-extended-classes-fixer branch from a299fe1 to 2866ba5 Compare August 23, 2019 14:35

grossmannmartin requested changes Aug 26, 2019

View reviewed changes

grossmannmartin reviewed Aug 26, 2019

View reviewed changes

docs/introduction/framework-extensibility.md Show resolved Hide resolved

grossmannmartin requested changes Aug 26, 2019

View reviewed changes

docs/introduction/framework-extensibility.md Outdated Show resolved Hide resolved

docs/introduction/framework-extensibility.md Outdated Show resolved Hide resolved

docs/introduction/framework-extensibility.md Outdated Show resolved Hide resolved

vitek-rostislav force-pushed the rv-extended-classes-fixer branch 4 times, most recently from 9cae3fe to a6c0c6b Compare August 27, 2019 16:22

henzigo reviewed Aug 27, 2019

View reviewed changes

docs/introduction/framework-extensibility.md Outdated Show resolved Hide resolved

vitek-rostislav force-pushed the rv-extended-classes-fixer branch 6 times, most recently from 76f040e to e8d18c2 Compare September 2, 2019 19:17

grossmannmartin requested changes Sep 2, 2019

View reviewed changes

grossmannmartin requested changes Sep 3, 2019

View reviewed changes

packages/framework/phpstan.neon Outdated Show resolved Hide resolved

phpstan.neon Outdated Show resolved Hide resolved

LukasHeinz previously requested changes Sep 3, 2019

View reviewed changes

vitek-rostislav mentioned this pull request Sep 3, 2019

Docs - we should recommend mirroring namespaces in project-base when extending framework classes #1366

Closed

vitek-rostislav force-pushed the rv-extended-classes-fixer branch 3 times, most recently from aceb253 to 6802951 Compare September 3, 2019 17:16

grossmannmartin requested changes Sep 3, 2019

View reviewed changes

grossmannmartin requested changes Sep 4, 2019

View reviewed changes

grossmannmartin approved these changes Sep 4, 2019

View reviewed changes

vitek-rostislav mentioned this pull request Sep 4, 2019

inline annotations for the interfaces - what is the correct way? #1369

Closed

vitek-rostislav added 8 commits September 4, 2019 13:15

FixExtendedClassesAnnotationsCommand: extraction of AnnotationsReplacer

70a38e0

- the class is unit tested - AnnotationsReplacementsMap was extracted as well because the mappings are used on multiple places in the command

FixExtendedClassesAnnotationsCommand: replaceInMethodReturnType() and…

596c463

… replaceInPropertyType() extracted to AnnotationsReplacer - added unit tests for the methods

FixExtendedClassesAnnotationsCommand: fixed finding relevant files fo…

0ae25da

…r annotations replacements - getRelevantFrameworkAnnotationsPattern() was broken by rewritting the history of the original commit - the functionality extracted to AnnotationsReplacementsMap::getPatternForAny

FixExtendedClassesAnnotationsCommand: use simpler way of creating a b…

005de86

…etter reflection class

"php phing fix-annotations" is applied on project-base

d0c0f73

- all the automated fixes and additions of annotations are applied - the standards are failing intentionally on this commit as there are several phpstan rules violations that need to be fixed manually yet

vitek-rostislav force-pushed the rv-extended-classes-fixer branch from afa2571 to 5ce5f88 Compare September 4, 2019 11:26

grossmannmartin requested changes Sep 4, 2019

View reviewed changes

packages/framework/src/Component/ClassExtension/ClassExtensionRegistry.php Outdated Show resolved Hide resolved

vitek-rostislav force-pushed the rv-extended-classes-fixer branch from bb02ba9 to c9d6429 Compare September 5, 2019 06:16

grossmannmartin approved these changes Sep 5, 2019

View reviewed changes

DavidOstrozlik approved these changes Sep 5, 2019

View reviewed changes

vitek-rostislav added 10 commits September 5, 2019 08:55

added upgrade note

99dece1

fixing annotations: added "dry-run" option to the command and more ve…

242bfdd

…rbose output - FixExtendedClassesAnnotationsCommand renamed to ExtendedClassesAnnotationsCommand - filenames that were fixed (or need to be fixed when using "dry-run" mode) are reported

added upgrade instruction for PHPStan level 4

0c80e67

fixed @method annotations in project

6069952

- parameter types are now taken into account as well

proper PhpDoc formatting of inline annotations that were affected by …

e3f90e4

…the annotations fixer - proper format is "/** @var Type $variableName */" above the variable - the annotations were malformatted before so we fixed what we noticed :)

phing targets "fix-annotations" and "check-annotations" renamed to "a…

94e190f

…nnotations-fix" and "annotations-check" - in accordance with our standards - https://github.com/shopsys/shopsys/blob/master/docs/contributing/guidelines-for-phing-targets.md#naming-conventions

console-commands-for-application-management-phing-targets.md: "annota…

22275f3

…tions-*" targets moved to "Coding Standards section"

ClassExtensionRegistry: extended controllers are now registered as well

64a1bf2

vitek-rostislav force-pushed the rv-extended-classes-fixer branch from 189f9db to 64a1bf2 Compare September 5, 2019 06:58

vitek-rostislav merged commit bd0ccc5 into master Sep 5, 2019

vitek-rostislav deleted the rv-extended-classes-fixer branch September 5, 2019 07:32

vitek-rostislav mentioned this pull request Sep 6, 2019

[project-base] resolved (either fixed or ignored) all the errors reported by phpstan level 4 #1381

Merged

PetrHeinz mentioned this pull request Sep 11, 2019

[project-base] Hack phpunit to make tests clean #1392

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[shopsys] automated fixes and additions of annotations for extended classes #1344

[shopsys] automated fixes and additions of annotations for extended classes #1344

vitek-rostislav commented Aug 22, 2019 •

edited

grossmannmartin left a comment

grossmannmartin left a comment

grossmannmartin left a comment

grossmannmartin left a comment

LukasHeinz left a comment

grossmannmartin left a comment

grossmannmartin left a comment

grossmannmartin left a comment

simara-svatopluk commented Sep 6, 2019

[shopsys] automated fixes and additions of annotations for extended classes #1344

[shopsys] automated fixes and additions of annotations for extended classes #1344

Conversation

vitek-rostislav commented Aug 22, 2019 • edited

TODO

grossmannmartin left a comment

Choose a reason for hiding this comment

grossmannmartin left a comment

Choose a reason for hiding this comment

grossmannmartin left a comment

Choose a reason for hiding this comment

grossmannmartin left a comment

Choose a reason for hiding this comment

LukasHeinz left a comment

Choose a reason for hiding this comment

grossmannmartin left a comment

Choose a reason for hiding this comment

grossmannmartin left a comment

Choose a reason for hiding this comment

grossmannmartin left a comment

Choose a reason for hiding this comment

simara-svatopluk commented Sep 6, 2019

vitek-rostislav commented Aug 22, 2019 •

edited