Upgrade from 2.8.x to 2.9.1: possible BC break for nullable behaviour

[jmsche]

Hi everyone,

I just upgraded doctrine/orm to 2.9.1 (from 2.8.x) on a simple project.

It makes some columns that were previously not nullable, nullable (the Column annotation does not have "nullable" set to any value in 2.9, but from what I can see it was false by default until 2.8.x included).

I think the changes happened since these PRs: #8439 & #8589

I guess the changes happen because the property in my code is nullable, and as nullable is not defined in my Column annotation, the driver thinks it's nullable.

Is it considered a BC break?

Regards,

[zolex]

I can confirm this behavior. also the JoinColumn(nullable=true) seems to be ignored and also the migrations creates update table statements that set nullable to false where it should be true.

[beberlei]

I thought I adressed this with #8678 - can you check how this relates?

[jmsche]

I think there are two different bugs here (but maybe related):

• Initial issue about @Column setting nullable to true
• @zolex's issue about @JoinColumn which can't override nullable

[dmaicher]

Similar problem here with 2.9.1 (upgraded from 2.8.5):

I have an entity with a relation like

/**
 * @ORM\Entity
 */
class InheritedQuestionnaire extends AbstractQuestionnaire
{
    /**
     * @ORM\JoinColumn(nullable=true)
     * @ORM\OneToOne(targetEntity="Questionnaire")
     */
    private Questionnaire $parentQuestionnaire;

    public function __construct(Questionnaire $parentQuestionnaire)
    {
        $this->parentQuestionnaire = $parentQuestionnaire;
    }
 
    // ...   
}

But since its using single table inheritance the column needs to be nullable. Only for this particular type its set. Now the nullable=true is ignored and I get a migration diff like

$this->addSql('ALTER TABLE questionnaire CHANGE parent_questionnaire_id parent_questionnaire_id INT NOT NULL');

[pieterw2w]

same issue as dmaicher: the column is no longer nullable.

[TheKassaK]

Same issue, downgraded to 2.8.5

[Lustmored]

@beberlei I'm afraid #8678 introduced at least one part of this problem. Change in lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php made it overwrite nullable attribute whenever non-nullable typed parameter is found.

This is because when nullable on JoinColumn annotation is non-nullable bool there is no way telling if it is being set by user or have default value and the overwrite happens every time.

Solving it the right way would probably require nullable on JoinColumn to be nullable again (and fighting strange bugs I didn't quite understood in other ORM parts) or using some other way to determine whether it was set or not or removing this rule and reintroduce it in 3.0 in some way that wouldn't enforce BC.

I can try to pursue "some other way to determine whether it was set or not" option but I am a bit concerned about mapping drivers that don't use JoinColumn class (YAML/XML/PHP) - I have to investigate them before judging.

[Lustmored]

I have created draft of the possible solution that minimizes BC impact and fixes overwriting nullable parameter. I would love getting some feedback before working on remaining drivers.

This however does not change default @Column behavior as it seems to defy it's purpose. Non-nullable typed property should in my opinion be not nullable in database by default, because passing null to non-nullable property throws TypeError as far as I know.

[dmaicher]

Non-nullable typed property should in my opinion be not nullable in database by default, because passing null to non-nullable property throws TypeError as far as I know.

@Lustmored but this is tricky when talking about single table entity inheritance as in my case mentioned above. There the column needs to be nullable for other types of the entity although its property is typed and not nullable

[Lustmored]

Non-nullable typed property should in my opinion be not nullable in database by default, because passing null to non-nullable property throws TypeError as far as I know.

@Lustmored but this is tricky when talking about single table entity inheritance as in my case mentioned above. There the column needs to be nullable for other types of the entity although its property is typed and not nullable

This is covered by my draft change where in your example column will still be nullable. The change is about defaults and it failed with JoinColumn, where defaults were enforced by mistake.

So your example:

/**
 * @ORM\Entity
 */
class InheritedQuestionnaire extends AbstractQuestionnaire
{
    /**
     * @ORM\JoinColumn(nullable=true)
     * @ORM\OneToOne(targetEntity="Questionnaire")
     */
    private Questionnaire $parentQuestionnaire;

    public function __construct(Questionnaire $parentQuestionnaire)
    {
        $this->parentQuestionnaire = $parentQuestionnaire;
    }
 
    // ...   
}

will work with my draft, while:

/**
 * @ORM\Entity
 */
class InheritedQuestionnaire extends AbstractQuestionnaire
{
    /**
     * @ORM\JoinColumn
     * @ORM\OneToOne(targetEntity="Questionnaire")
     */
    private Questionnaire $parentQuestionnaire;

    public function __construct(Questionnaire $parentQuestionnaire)
    {
        $this->parentQuestionnaire = $parentQuestionnaire;
    }
 
    // ...   
}

will still default to nullable: false).

Hope that makes it clear :) I'm no expert here, so possibly this approach have some drawbacks too.

[beberlei]

I will look into this and @Lustmored draft PR the coming days, this has highest priority to fix with a release planned for next week.

[Lustmored]

I will look into this and @Lustmored draft PR the coming days, this has highest priority to fix with a release planned for next week.

I can manage time to work on this PR on Monday, so if idea behind it seems right let me know and I'll finish it.

[beberlei]

I think we need to remove the nullable/ˋallowsNullˋ code completely, because as said you might want toconfigure this differently in DB and code

[Lustmored]

@beberlei How do you feel about leaving this logic only for Attribute driver? It is new in 2.9 (no BC break), requires PHP8 (so typed properties will be common there) and could simply be useful to new users. I could get working PR today.

Otherwise - is this change something you'd like to see for ORM 3?

[zolex]

@beberlei How do you feel about leaving this logic only for Attribute driver? It is new in 2.9 (no BC break), requires PHP8 (so typed properties will be common there) and could simply be useful to new users. I could get working PR today.

Otherwise - is this change something you'd like to see for ORM 3?

I don't think it's good idea to have different logic for the same definitions in annotations and attributes.

[beberlei]

I agree with @zolex the logic should be consistent across drivers. We could look into this for ORM 3 though. At least if nullable is not set and the field allows null, then nullable should default to true. A user could always change it back to be nullable=false.

[Lustmored]

I agree with @zolex the logic should be consistent across drivers. We could look into this for ORM 3 though. At least if nullable is not set and the field allows null, then nullable should default to true. A user could always change it back to be nullable=false.

All right, thanks for the answer. I will work towards PR targeting 3.0 👍