Restore bc for annotations

[greg0ire]

Some doubts I have about this:

• should deprecations be contributed to 2.10.x instead?
• are there more things to handle with deprecations (more require arguments, and more classes OneToOne?)

[greg0ire]

@albe please review

[SenseException]

Why adding nullable when it gets deprecated?

[greg0ire]

See the commit message :)

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

[albe]

Thanks a lot! Looks good from my side, but haven't yet checked against Neos Flow if that makes things fully work. Would need to find some more time first.

should deprecations be contributed to 2.10.x instead?

IMO as a consumer, yes, as right now that will create LOTS of warnings. But I do see how the deprecation conceptually already happened with introduction of the constructor in 2.9.0 and those deprecations are already included in some other annotation classes - sooo... guess that's what it is?

are there more things to handle with deprecations (more require arguments, and more classes OneToOne?)

Other required arguments (no default value) are in:

• ChangeTrackingPolicy
• InheritanceType
• OrderBy

[beberlei]

This looks good! 🚢

Deprecations are ok for 2.9 from my POV, especially they don't generate a lot messages as per doctrine/deprecations contract (not using trigger_error).

[beberlei]

@albe what do you mean other required arguments are in OrderBy et al? Do they caus eproblems for Neos/Flow? If not, then some required arguments are fine imho.

[albe]

No, not causing issues for us. Was just looking through all Annotation classes that introduced additional required arguments as @greg0ire asked in the PR message

[greg0ire]

I guess if it's not causing issues for you, it's fine to have that "BC break" stay.

[greg0ire]

Not sure which label to use here…

[mbabker]

@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?

[greg0ire]

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?

[mbabker]

#8439 was the base PR and there are a couple follow-ons after it.

[greg0ire]

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.

[mbabker]

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'].

[greg0ire]

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.

[mbabker]

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.

[greg0ire]

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).

[greg0ire]

Working on it at #8784 🙂

[greg0ire]

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 😅