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.

Sign up for GitHub

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

Restore bc for annotations #8758

Merged
merged 2 commits into from
Jun 10, 2021
Merged

Restore bc for annotations #8758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
303e346
Restore backwards-compatibility
greg0ire Jun 9, 2021
10e41ec
Deprecate required of mandatory arguments
greg0ire Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/Doctrine/ORM/Mapping/Embedded.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

use Attribute;
use Doctrine\Common\Annotations\Annotation\NamedArgumentConstructor;
use Doctrine\Deprecations\Deprecation;

/**
* @Annotation
Expand All @@ -39,6 +40,14 @@ final class Embedded implements Annotation

public function __construct(?string $class = null, $columnPrefix = null)
{
if ($class === null) {
Deprecation::trigger(
'doctrine/orm',
'https://github.com/doctrine/orm/issues/8753',
'Passing no class is deprecated.'
);
}

$this->class = $class;
$this->columnPrefix = $columnPrefix;
}
Expand Down
13 changes: 11 additions & 2 deletions lib/Doctrine/ORM/Mapping/ManyToMany.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

use Attribute;
use Doctrine\Common\Annotations\Annotation\NamedArgumentConstructor;
use Doctrine\Deprecations\Deprecation;

/**
* @Annotation
Expand All @@ -31,7 +32,7 @@
#[Attribute(Attribute::TARGET_PROPERTY)]
final class ManyToMany implements Annotation
{
/** @var string */
/** @var string|null */
public $targetEntity;

/** @var string */
Expand Down Expand Up @@ -61,14 +62,22 @@ final class ManyToMany implements Annotation
* @param array<string> $cascade
*/
public function __construct(
string $targetEntity,
?string $targetEntity = null,
Copy link
Member

Choose a reason for hiding this comment

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

Why adding nullable when it gets deprecated?

Copy link
Member Author

Choose a reason for hiding this comment

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

See the commit message :)

There used to be no constructor in this class, adding one with mandatory
arguments was technically a BC-break.

?string $mappedBy = null,
?string $inversedBy = null,
?array $cascade = null,
string $fetch = 'LAZY',
bool $orphanRemoval = false,
?string $indexBy = null
) {
if ($targetEntity === null) {
Deprecation::trigger(
'doctrine/orm',
'https://github.com/doctrine/orm/issues/8753',
'Passing no target entity is deprecated.'
);
}

$this->targetEntity = $targetEntity;
$this->mappedBy = $mappedBy;
$this->inversedBy = $inversedBy;
Expand Down
9 changes: 9 additions & 0 deletions lib/Doctrine/ORM/Mapping/ManyToOne.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

use Attribute;
use Doctrine\Common\Annotations\Annotation\NamedArgumentConstructor;
use Doctrine\Deprecations\Deprecation;

/**
* @Annotation
Expand Down Expand Up @@ -57,6 +58,14 @@ public function __construct(
string $fetch = 'LAZY',
?string $inversedBy = null
) {
if ($targetEntity === null) {
Deprecation::trigger(
Copy link
Contributor

Choose a reason for hiding this comment

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

@greg0ire I realize I'm a little late to the party on this one, but I just noticed the deprecation in our dev logs today.

One of the features of 2.9 was automatic type detection when using typed properties, but these deprecations imply that with ORM 3.0 that the $targetEntity param will be required which negates the automatic type detection feature. Is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how type detection works, but it looks kind of strange to me that the build passed prior to this, with a required constructor argument. Maybe type detection is done before building this class? If not, then something is off. Can you please find the PR which introduced automatic type detection?

Copy link
Contributor

Choose a reason for hiding this comment

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

#8439 was the base PR and there are a couple follow-ons after 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.

I could be wrong, but from this PR you linked, it looks like the detection happens in order to complete the mapping and make it valid (provide that $targetEntity argument) before instanciating ManyToOne rather than changing a ManyToOne instance after the fact. Also, I don't think we got complaints about this when the version with mandatory arguments was released. And also, no tests broke.

I'll stop the investigation here, if you still think it negates the automatic type detection feature, please send a failing test.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ManyToMany annotation was the only one that had the required constructor argument, which this PR did address from the B/C standpoint.

But, this PR also adds deprecations when $targetEntity === null for @Embedded (which just added type detection in #8724), @ManyToMany, and @ManyToOne, which the automatic type detection feature relies on as a DX improvement (basically, we can omit the target when using PHP 7.4+ and typed properties, the metadata mapping would infer it from the property type). So my reading on the deprecations would be the automatic type detection feature goes away with ORM 3 because the $targetEntity argument would be required as basically ClassMetadataInfo::validateAndCompleteTypedAssociationMapping() would never hit the check to assign $mapping['targetEntity'].

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm under the impression that you think that $mapping is an object… the classes we are talking about do not implement ArrayAccess AFAIK, so the $mapping['targetEntity'] should make you think that it is in fact an array. I also think that #8724 will make the deprecation you are observing in your logs disappear for Embedded.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I'm clearly not communicating my concern very well.

I have these two simplified entities in my app:

<?php declare(strict_types=1);

namespace App\Entity;

use Doctrine\ORM\EntityRepository;
use Doctrine\ORM\Mapping as ORM;

#[ORM\Entity(repositoryClass: EntityRepository::class)]
#[ORM\Table]
class UserDocument
{
    #[ORM\Column(nullable: false)]
    #[ORM\Id]
    #[ORM\GeneratedValue]
    private ?int $id = null;

    #[ORM\ManyToOne(cascade: ['persist', 'remove'], inversedBy: 'documents')]
    #[ORM\JoinColumn(nullable: false, onDelete: 'CASCADE')]
    public User $user;

    public function __construct(User $user)
    {
        $this->user = $user;
    }

    public function getId(): ?int
    {
        return $this->id;
    }
}
<?php declare(strict_types=1);

namespace App\Entity;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\EntityRepository;
use Doctrine\ORM\Mapping as ORM;

#[ORM\Entity(repositoryClass: EntityRepository::class)]
#[ORM\Table]
class User
{
    #[ORM\Column(nullable: false)]
    #[ORM\Id]
    #[ORM\GeneratedValue]
    private ?int $id = null;

    /**
     * @var Collection<UserDocument>
     */
    #[ORM\OneToMany(mappedBy: 'user', targetEntity: UserDocument::class, cascade: ['persist'], fetch: 'EXTRA_LAZY')]
    private Collection $documents;

    public function __construct()
    {
        $this->documents = new ArrayCollection();
    }

    public function getId(): ?int
    {
        return $this->id;
    }

    public function addDocument(UserDocument $document): void
    {
        $this->documents->add($document);
    }

    public function removeDocument(UserDocument $document): void
    {
        $this->documents->removeElement($document);
    }

    /**
     * @return Collection<UserDocument>
     */
    public function getDocuments(): Collection
    {
        return $this->documents;
    }

    public function hasDocument(UserDocument $document): bool
    {
        return $this->documents->contains($document);
    }

    public function getDocumentCount(): int
    {
        return $this->documents->count();
    }
}

This triggers the "Passing no target entity is deprecated. (ManyToOne.php:65 called by UserDocument.php:17, #8753, package doctrine/orm)" deprecation message. The mapping works fine now because of the automatic type detection feature (ultimately ClassMetadataInfo handles assigning the target entity correctly because I have a typed property), but I am being told that not passing the targetEntity parameter is deprecated, which is the exact thing that the automatic type detection feature does for me.

Do I need to restore the targetEntity parameter on all of my mapping attributes or not? Based on the deprecation coming from the ManyToOne class constructor, I am being told yes, but as I said in my original comment, that implies to me that the automatic type detection gets cancelled out by requiring the annotation to be configured.

Copy link
Member Author

@greg0ire greg0ire Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ah yes it makes more sense to me now, and I see that logically, a ManyToOne object exists to build the mapping, not the other way around (since there are other drivers and all are meant to build the same kind of mapping array). I think you are right, the deprecations should be removed. I clearly did not have that autodetection feature in mind when writing #8753 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

Working on it at #8784 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'm clearly not communicating my concern very well.

Thanks for being patient, I'm unfamiliar with the auto-detection feature and the fact that it's only implemented for some of the relationships did not help with understanding 😅

'doctrine/orm',
'https://github.com/doctrine/orm/issues/8753',
'Passing no target entity is deprecated.'
);
}

$this->targetEntity = $targetEntity;
$this->cascade = $cascade;
$this->fetch = $fetch;
Expand Down