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

Add native type to private properties and final classes #2666

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jul 1, 2024

Q A
Type improvement
BC Break no
Fixed issues -

Summary

Some private properties were not typed, mainly because mixed or union types were not available in previous PHP version.

@GromNaN GromNaN added this to the 2.9.0 milestone Jul 1, 2024
@GromNaN GromNaN force-pushed the union-type branch 3 times, most recently from fe5b616 to a87f55a Compare July 1, 2024 20:13
@@ -64,23 +56,6 @@ public function __construct(DocumentManager $dm, UnitOfWork $uow)
{
$this->dm = $dm;
$this->uow = $uow;

$this->defaultPrimer = static function (DocumentManager $dm, ClassMetadata $class, array $ids, array $hints): void {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to private class method.

@GromNaN GromNaN marked this pull request as ready for review July 1, 2024 20:14
@@ -124,7 +99,7 @@ public function primeReferences(ClassMetadata $class, $documents, string $fieldN
throw new LogicException(sprintf('Field "%s" is an identifier reference without a target document class in class "%s"', $fieldName, $class->name));
}

$primer = $primer ?: $this->defaultPrimer;
$primer = $primer ?: $this->defaultPrimer(...);
Copy link
Member Author

Choose a reason for hiding this comment

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

First-class closure available since PHP 8.1

@@ -32,8 +32,7 @@
*/
class Fill extends Stage
{
/** @var mixed|Expr|null */
private $partitionBy = null;
private mixed $partitionBy = null;
Copy link
Member

Choose a reason for hiding this comment

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

Dunno how much value it's bringing IRL, but knowing there could be Expr makes one remember about it

Copy link
Member Author

Choose a reason for hiding this comment

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

This is reported by phpcs.

FILE: lib/Doctrine/ODM/MongoDB/Aggregation/Stage/Fill.php
-----------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------
 35 | ERROR | [x] Property
    |       |     \Doctrine\ODM\MongoDB\Aggregation\Stage\Fill::$partitionBy has
    |       |     useless @var annotation.
-----------------------------------------------------------------------------------

I could try to restrict the types, but I would have to accept object which also overlaps Expr.

This is a private property, the setter keeps the full documented types:

/** @param mixed|Expr $expression */
public function partitionBy($expression): static

@@ -24,8 +24,8 @@
*/
final class HydratingIterator implements Iterator
{
/** @var Generator<mixed, array<string, mixed>>|null */
private $iterator;
/** @var Generator<mixed, array<string, mixed>> */
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't |null stay here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I tried to make it non-nullable and unset it in __destruct, but that didn't work as expected.

* @param mixed[] $ids
* @param array<int, mixed> $hints
*/
private function defaultPrimer(DocumentManager $dm, ClassMetadata $class, array $ids, array $hints): void
Copy link
Member

Choose a reason for hiding this comment

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

should this remain static?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, so we are sure we don't access the ReferencePrimer instance.

@alcaeus alcaeus self-requested a review July 2, 2024 08:07
@GromNaN GromNaN force-pushed the union-type branch 2 times, most recently from d2bf406 to e21c473 Compare July 2, 2024 10:18
@GromNaN GromNaN requested a review from malarzm July 2, 2024 10:19
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

LGTM with a minor question, but fine to merge as is.

*/
private $dm;
private ?DocumentManager $dm = null;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how exactly this property is used in this code, but would it make more sense to rely on the TypeError if it wasn't properly initialised before access?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the nullable, it seems to work.

*/
private $uow;
private ?UnitOfWork $uow = null;
Copy link
Member

Choose a reason for hiding this comment

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

Same question as with the dm property above

@GromNaN GromNaN force-pushed the union-type branch 2 times, most recently from 4d9740a to 4fd4cf1 Compare July 2, 2024 10:53
@GromNaN GromNaN enabled auto-merge (squash) July 2, 2024 10:54
@GromNaN GromNaN merged commit e9f8083 into doctrine:2.9.x Jul 2, 2024
4 of 17 checks passed
@GromNaN GromNaN deleted the union-type branch July 2, 2024 11:44
alcaeus added a commit that referenced this pull request Sep 6, 2024
* 2.9.x: (24 commits)
  Fix typo in code example (#2670)
  Label PRs about GH actions with "CI" (#2632)
  Review basic mapping (#2668)
  Fix wording (#2667)
  Add native type to private properties and final classes (#2666)
  Review and add tests on `ResolveTargetDocumentListener` (#2660)
  Remove soft-delete-cookbook (#2657)
  doc: Remove wakeup and clone cookbook (#2663)
  Modernize generated code for Hydrators (#2665)
  Add tests for introduction (#2664)
  doc: Review mapping ORM and ODM cookbook (#2658)
  doc: Review cookbook on blending ORM and ODM (#2656)
  doc: Review and test validation cookbook (#2662)
  Update custom mapping example (#2654)
  doc: Review Simple Search Engine Cookbook (#2659)
  doc: Add cookbook about embedding referenced documents using $lookup (#2655)
  doc: Add type to properties (#2652)
  doc: Review custom collections and repository docs (#2653)
  doc: Review Getting Started (#2650)
  Move annotations-reference to attributes-reference (#2651)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants