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

Better doc typing #7049

Merged
merged 3 commits into from
Mar 31, 2023
Merged

Better doc typing #7049

merged 3 commits into from
Mar 31, 2023

Conversation

mamazu
Copy link
Contributor

@mamazu mamazu commented Mar 31, 2023

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? don't write untyped code :)
Fixed tickets -
Related issues/PRs -
License MIT
Documentation PR -

What's in this PR?

  • Adding some types in doc comments
  • Improving types in some tests
  • Adding explicit null return for non void functions (not a bc break)
  • Updated some incorrect doctypes

Why?

As preparation for 3.0 we want the doc types to be as correct as possible for tools to rely on them when refactoring.

@mamazu mamazu changed the base branch from 2.5 to 2.6 March 31, 2023 10:03
*/
public function testFindPortalInformationByUrlWithInvalidSuffix($url, $shouldFind): void
public function testFindPortalInformationByUrlWithInvalidSuffix(string $url, $shouldFind): void
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 is not a bc break because the function where those variables are used already have a return type of array. So the only thing is we fail the code faster on broken types.

Copy link
Member

Choose a reason for hiding this comment

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

yeah normally parameters types are bc breaks when class is not final but in tests we don't need to take care about bc and can set them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So in tests "BC breaking behavior" (like adding parameters and return types) is okay in test methods?

Copy link
Member

Choose a reason for hiding this comment

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

@mamazu yes totally in tests we don't need to take care of the bc breaks, only the Testing namespace of the SuluTestBundle is the only one which part of the public namespace.

We already added and fixed some types in tests via rector here:

If we do that kind of massive manipulation I would prefer todo it on 2.4 branch or for PHP 8.0 changes on 2.5 avoids that we have problems in backmerge a bugfix with a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will keep that in mind for further type changes.

@alexander-schranz
Copy link
Member

alexander-schranz commented Mar 31, 2023

PHP 8.2 is unrelated not sure yet whats wrong with gedmo @dev something or doctrine/orm 2.15.x.

@alexander-schranz
Copy link
Member

PHP 8.1 should not fail there seems to be something wrong.

@mamazu
Copy link
Contributor Author

mamazu commented Mar 31, 2023

Fixed and asked a question on the last remaining thread.

@mamazu
Copy link
Contributor Author

mamazu commented Mar 31, 2023

Then this MR is ready for review

@alexander-schranz alexander-schranz merged commit f2a1554 into sulu:2.6 Mar 31, 2023
@mamazu mamazu deleted the better_doc_typing branch March 31, 2023 16:13
@alexander-schranz
Copy link
Member

@mamazu Thank you! ~400 less lines in phpstan baseline 💪

mamazu added a commit to mamazu/sulu that referenced this pull request Apr 4, 2023
mamazu added a commit to mamazu/sulu that referenced this pull request Apr 4, 2023
mamazu added a commit to mamazu/sulu that referenced this pull request Jun 17, 2023
mamazu added a commit to mamazu/sulu that referenced this pull request Jun 22, 2023
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

2 participants