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

Type declarations to increase phpstan to level 6 #591

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Sep 11, 2022

  1. Bump dependencies in composer.json

  2. Bump phpstan to level 6

  3. WIP - fixup type declarations (test code)

  4. ToDo - fixup type declarations (real code)

Towards issue #588

@phil-davis phil-davis self-assigned this Sep 11, 2022
@codecov
Copy link

codecov bot commented Sep 11, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (154a84d) 98.76% compared to head (18494f5) 98.68%.

❗ Current head 18494f5 differs from pull request most recent head 6f8d36e. Consider uploading reports for the commit 6f8d36e to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #591      +/-   ##
============================================
- Coverage     98.76%   98.68%   -0.08%     
+ Complexity     1866     1865       -1     
============================================
  Files            71       71              
  Lines          5325     5329       +4     
============================================
  Hits           5259     5259              
- Misses           66       70       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phil-davis
Copy link
Contributor Author

"progress" report: phpstan [ERROR] Found 638 errors

There were 880 errors, I have fixed 242 - that is 27.5% done! This is a painful job.

@@ -160,7 +160,7 @@ public static function guessParameterNameByValue(string $value): string
*
* This may be either a single, or multiple strings in an array.
*
* @param string|array $value
* @param int|string|array $value
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to define more precise array-key and value types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably - I will look in more detail when I get to do a lot a "real code".

@@ -48,8 +51,13 @@ public function testParseTz(): void
END:VEVENT
END:VCALENDAR';

/** @var VCalendar<int, mixed> $vObject */
Copy link
Member

Choose a reason for hiding this comment

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

can we define a more precise return-type instead?

public static function read($data, int $options = 0, string $charset = 'UTF-8'): ?Document

@var has the problem, that we need to declare it on every call-site

Comment on lines 55 to 62
$vcard->PHOTO = 'random_stuff';
/* @phpstan-ignore-next-line 'Cannot call method add() on string' */
$vcard->PHOTO->add(null, 'BASE64');
Copy link
Member

Choose a reason for hiding this comment

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

how can we call ->add on a property, which we assigned a string shortly before?

is this related to magic __get & __set or similar, and we should/can use a extension in phpstan for that?
https://phpstan.org/developing-extensions/class-reflection-extensions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, there is "magic property" stuff happening, so PHOTO does not really end up being a string!
I haven't dug deep into this. I was not being too concerned about the test code.
But I will probably have to dig into it when I get to the "real" code.

@phil-davis
Copy link
Contributor Author

Types are declared in all test code.
Found 493 errors
Nearly half way. And then need to review what happened and try to tidy it all up, be consistent, and provide the "reasonable most" specific type declarations at each point.

@phil-davis
Copy link
Contributor Author

Note to interested people: I have had "other things" happening the last few months. This work here is volunteer work. Hopefully I will get enthusiastic about it again "real soon now" (tm) and have some time to put in.

If anyone else wants to do this stuff, or help with it, feel free to ping me here and we can work out how to move forward.

@phil-davis
Copy link
Contributor Author

phil-davis commented Jan 23, 2023

Rebased and resolved conflicts. Maybe I can get more work done on this "soon".

composer phpstan
...
Found 541 errors

@phil-davis
Copy link
Contributor Author

@phil-davis
Copy link
Contributor Author

I rebased, resolved conflicts, applied the latest cs-fixer code-style changes. Now phpstan level 6 reports "only":

[ERROR] Found 382 errors

This looks like a big job.

@DeepDiver1975
Copy link
Member

I tried to address some stuff here as well
https://github.com/sabre-io/vobject/pull/628/commits

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.

3 participants