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

Finish adding return type declarations to phpdoc #2368

Merged
merged 3 commits into from
Oct 23, 2021

Conversation

franmomu
Copy link
Contributor

Q A
Type improvement
BC Break no
Fixed issues

Summary

This is the last PR adding return type declarations (I'll create a couple for parameter type declarations), there is one change in GraphLookup:: convertTargetFieldName, this method is called in:

public function connectFromField(string $connectFromField): self
{
// No targetClass mapping - simply use field name as is
if (! $this->targetClass) {
$this->connectFromField = $connectFromField;
return $this;
}
// connectFromField doesn't have to be a reference - in this case, just convert the field name
if (! $this->targetClass->hasReference($connectFromField)) {
$this->connectFromField = $this->convertTargetFieldName($connectFromField);
return $this;
}

and

public function connectToField(string $connectToField): self
{
$this->connectToField = $this->convertTargetFieldName($connectToField);
return $this;
}

In both calls the argument is a string, so it will never be an array.

@franmomu franmomu added the Task label Sep 14, 2021
@franmomu franmomu requested a review from malarzm September 16, 2021 07:26
@@ -44,6 +44,9 @@ public function toArray(): array
return iterator_to_array($this);
}

/**
* {@inheritDoc}
Copy link
Member

Choose a reason for hiding this comment

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

why do we need these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the inheritDoc cases, I was getting these errors:

FILE: lib/Doctrine/ODM/MongoDB/Iterator/PrimingIterator.php
-----------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------------------------
 47 | ERROR | Method \Doctrine\ODM\MongoDB\Iterator\PrimingIterator::current() does not have return type hint nor @return annotation for its return value.
-----------------------------------------------------------------------------------------------------------------------------------------------------------

I remember reading about phpcs that performs analysis by file, so it doesn't get that information from the parent, I'll see if there is a way to avoid this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

slevomat/coding-standard#1149 🤔 the only thing I can think of is that we can try to reach level 5 of phpstan that checks missing type declarations and remove this sniff.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Level 6 sorry 😞

Copy link
Member

Choose a reason for hiding this comment

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

Dang, so close. Personally I'd rather not clutter the code with useless inheritdoc just to satisfy CS Fixer. How about merging all the other changes, still keep the rule as excluded, and work towards level 6? I'd also much appreciate @alcaeus input on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like inheritdoc either, I'm fine keeping the rule excluded. Should I remove all the inheritdoc? (apart from the ones added in this PR) 🤔

Copy link
Member

Choose a reason for hiding this comment

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

👍 for removing all @inheritDocs

lib/Doctrine/ODM/MongoDB/Query/Expr.php Outdated Show resolved Hide resolved
@franmomu franmomu requested a review from malarzm October 13, 2021 11:35
@franmomu franmomu added this to the 2.3.0 milestone Oct 13, 2021
This will avoid deprecations with Symfony 5.4
@franmomu
Copy link
Contributor Author

friendly ping @malarzm if you could take a look, the @inheritDoc blocks have been removed

PS: if you have more time (and feel like reviewing) you can also take a look at #2369 😬

@malarzm malarzm merged commit 6cb1e7f into doctrine:2.3.x Oct 23, 2021
@malarzm
Copy link
Member

malarzm commented Oct 23, 2021

Thanks @franmomu and sorry for keeping you waiting 🎉

@franmomu franmomu deleted the add_types_5_2 branch October 25, 2021 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants