Set `JoinColumn::referencedColumnName` based on the target entity instead of `id`

[kiler129]

Feature Request

Q	A
New Feature	yes
RFC	yes
BC Break	yes/no

Summary

When relationships are defined the referencedColumnName is automatically derived from the target entity, unless JoinColumn is specified. When JoinColumn is added for any other purpose the referencedColumnName is hardcoded to id. I think it should be fetched from the target entity as if no JoinColumn was defined.

Reasoning

When defining relationships, e.g.:

    #[ORM\OneToOne(inversedBy: 'media')]
    #[ORM\JoinColumn(name: 'ZATTACHMENT1')]
    public Attachment $attachment;

A lot of things are assumed:

• type of the relationship from PHP type
• nullability
• target entity (based on type)
• column name (based on property name transformed by naming strategy via joinColumnName())
• reference column name on the other side (based on target entity PK, optionally derived from naming strategy referenceColumnName())

Normally entities also get their primary key column name from the naming strategy, making the JoinColumn optional. However, as soon as it gets added for a different purpose (like above: to change column name) it creates a strange side effect of referencedColumnName being overridden by a hardcoded value of id:

https://github.com/doctrine/orm/blob/84df37de97d5cdac545f4070ec90a7279f806949/lib/Doctrine/ORM/Mapping/JoinColumn.php#L43-L45

Possible improvements

I'm sure there was a reason why this decision was made, but I cannot find it.

The ideal solution, in my opinion, will be defaulting the referencedColumnName to the target entity primary key, in the same was as when JoinColumn isn't present on the property. However, this may be seen as a BC break. Changing it now to suddenly read from the naming strategy/target entity may not be the best course of action as applications may rely on this behavior (even if a small number).

However, it's also extremely unintuitive (especially when using PHPDoc-based annotations) to see the referenced column change when the user think adding JoinColumn is added to only change the column name. The alternative could be explicitly setting the parameter to null to indicate "figure this out yourself". Currently it's impossible to achieve that - one has to manually specify referencedColumnName every time JoinColumn is used.

WDYT? Can this be improved? Currently it leads to a lot of boilerplate code and semi-hardcoded PK name outside of a custom naming strategy.

[curry684]

Related to this I just ran into a similarly annoying issue - the fact that 'id' is hardcoded as the referenced column name means that naming strategy is bypassed. I have a project that, for legacy reasons, has a naming strategy with leading caps:

    /**
     * Returns the default reference column name.
     *
     * @return string A column name
     */
    public function referenceColumnName(): string
    {
        return 'Id';
    }

    /**
     * Returns a join column name for a property.
     *
     * @param string $propertyName A property name
     *
     * @return string A join column name
     */
    public function joinColumnName($propertyName)
    {
        return ucfirst($propertyName) . $this->referenceColumnName();
    }

Now not adding a JoinColumn attribute works fine, however when I want to make the join not nullable:

#[JoinColumn(nullable: false)]

I get a mapping violation:

 * The referenced column name 'id' has to be a primary key column on the target entity class '...'.

The fix for this is identical to what @kiler129 mentions above - the default should not be 'id' but NULL, indicating "please figure out a sane default using the name within the configured naming strategy".

[k00ni]

Any news on this?

[siggidiel]

Same here. We have a custom naming strategy which prefixes all columns of a table with the table name. So, also the ID column ends up with the name tableb_id. And if we have a relation from tablea to tableb, we are providing the name like this: tablea_tableb_id just to directly show that this column is a foreign key to TableB.

But inside the entity TableA (owning side) the annotation/attribute JoinColumn(referenceColumnName: 'tableb_id') needs to be set explicitly because the ID string is hardcoded as already mentioned by the thread creator. So, because the method public function referenceColumnName(): string from the NamingStrategy interface does not provide any method arguments to dynamically format your reference column name, it's always necessary to explicitly set the annotation in the entities.

[curry684]

As the current behaviour is downright broken I do think something should be done about it, however technically the change is breaking, even though I do not think there's any code out there relying on the id default and having a custom naming strategy, so it will not actually break any real world code.

The fix is fairly easy but, knowing Doctrine's stance on BC, it will likely have to await a new major.

[greg0ire]

Possible next steps:

• provide a PHPUnit test allowing to reproduce the bug
• provide a naive, BC-breaking fix demonstrating what we should end up with in 4.0.0

When that's done, we can try to imagine ways to provide the same fix without a breaking change, like maybe introducing a new attribute just for this, or a temporary extra argument that you could invoke like so: #[ORM\JoinColumn(name: 'ZATTACHMENT1', detect_pk: true)]

[curry684]

Adding it to every JoinColumn would hardly be better than just adding the referencedColumnName :)

I would prefer a config level switch to fix the behavior in a minor and flip the default in ORM 4, since having a naming strategy is also done there:

doctrine:
    orm:
        naming_strategy: My\Project\Doctrine\ORM\MyNamingStrategy
        references_follow_naming_strategy: true

[greg0ire]

Yeah that would be great indeed! To that end, it would be great to locate the piece of code where the autodetection happens.

[curry684]

I think the fix itself is easier than anticipated: https://github.com/curry684/orm/commit/722969909b7d17e8450683e38b94978a36794ddd

Is there a timeline on ORM4? Ie. - is it even worth implementing a B/C layer (which would be far more work and involve DoctrineBundle as well)

[greg0ire]

No clear timeline yet, right now we're trying to get ORM 3 adopted, it is still young. I can't imagine either how it would break existing code, so I'd say let's target 3.4.x, and be prepared to revert this upon release if we see complaints / lectures about semver and whether we know what it means.

[curry684]

Ok, in that case I'll see if I can whip up some tests for the change :)

(most important test first: I'll symlink this build into the one project I have with a custom strategy)

[curry684]

Tests added, seems to work fine for all drivers.

(most important test first: I'll symlink this build into the one project I have with a custom strategy)

This test also succeeded - copying my PR build of ORM into the project and removing the referencedColumnName declarations from all JoinColumn attributes results in an empty changeset to the database without errors.

PR targets 3.4 as suggested above.

[curry684]

For completion's sake: this issue is only partially resolved by my PR as @ServerExe in https://github.com/doctrine/orm/issues/9558#issuecomment-2553386677 requests functionality to determine the referencedColumnName based on the relation. As this requires changing the NamingStrategy interface it's highly breaking and can only be done in a major.

The change is made a lot easier by my PR though if someone wants to pick up from here.

[siggidiel]

For completion's sake: this issue is only partially resolved by my PR as @ServerExe in https://github.com/doctrine/orm/issues/9558#issuecomment-2553386677 requests functionality to determine the referencedColumnName based on the relation. As this requires changing the NamingStrategy interface it's highly breaking and can only be done in a major.

The change is made a lot easier by my PR though if someone wants to pick up from here.

Hey @curry684,

thanks for your response. It isn't necessarily a request of mine. I would be fine with other solutions. As long as the referenced column name can be determined somehow, I would appreciate it :) But I am also fine with a change in the next Major update. In the meantime, we can live with explicitly providing the name in the PHP Attribute.

[curry684]

To be honest I think, given how exotic custom naming strategies are, likely only being used for legacy databases, I don't think this request is going to get much love from the team for the next major.

Technically, we could add an interface parameter in PHP without B/C issues due to how PHP handles extra parameters in function calls, so you could attempt an implementation.