Use array shapes where appropriate

[greg0ire]

Working on converting these array shapes to DTO allowed me to find every signature where they are supposed to be used.

The Psalm baseline gets worse because it considers accessing an array key differently depending on whether it is defined vaguely, as array<string, mixed>, or precisely, as array{my-key?: string}.

See https://psalm.dev/r/5940ba233e

[greg0ire]

The most Psalm errors occur in this file, because Psalm does not know that joinTable is always set for mappings that represent the owning side of a many to many relationship.

As it stands, #10405 won't fix that either, because I did not create a separate type for owning sides, but maybe I should?

Before

classDiagram
    AssociationMapping <|-- OneToOneAssociationMapping
    AssociationMapping <|-- OneToManyAssociationMapping
    AssociationMapping <|-- ManyToOneAssociationMapping
    AssociationMapping <|-- ManyToManyAssociationMapping

Loading

After

For ManyToOne and for OneToMany, isOwningSide is redundant according to the docs. I think this could be modeled with an interface (not implementing it would mean the relationship is owned).

classDiagram
    AssociationMapping <|-- OneToOneAssociationMapping
    AssociationMapping <|-- OneToManyAssociationMapping
    AssociationMapping <|-- ManyToOneAssociationMapping
    AssociationMapping <|-- ManyToManyAssociationMapping
    ManyToManyAssociationMapping <|-- OwningManyToManyAssociationMapping
    ManyToManyAssociationMapping <|-- OwnedManyToManyAssociationMapping
    OneToOneAssociationMapping <|-- OwningOneToOneAssociationMapping
    OneToOneAssociationMapping <|-- OwnedOneToOneAssociationMapping

Loading

Cc @mpdude

[mpdude]

How could/would Psalm know when/where you're dealing with an AssociationMapping that represents the owning side? Would have have to make instanceof checks?

[greg0ire]

Yes, exactly! We would replace the guard check here, for instance:

orm/lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php

Lines 37 to 46 in 31ff969

	if (! $mapping['isOwningSide']) {
	return; // ignore inverse side
	}
	$types = [];
	$class = $this->em->getClassMetadata($mapping['sourceEntity']);
	foreach ($mapping['joinTable']['joinColumns'] as $joinColumn) {
	$types[] = PersisterHelper::getTypeOfColumn($joinColumn['referencedColumnName'], $class, $this->em);
	}

Looking at this, you can see how the owning and owned sides are very different in the case of the many to many relationship:

orm/lib/Doctrine/ORM/Mapping/ClassMetadata.php

Lines 1802 to 1886 in 0f77181

	if ($mapping['isOwningSide']) {
	// owning side MUST have a join table
	if (! isset($mapping['joinTable']['name'])) {
	$mapping['joinTable']['name'] = $this->namingStrategy->joinTableName($mapping['sourceEntity'], $mapping['targetEntity'], $mapping['fieldName']);
	}
	$selfReferencingEntityWithoutJoinColumns = $mapping['sourceEntity'] === $mapping['targetEntity']
	&& (! (isset($mapping['joinTable']['joinColumns']) || isset($mapping['joinTable']['inverseJoinColumns'])));
	if (! isset($mapping['joinTable']['joinColumns'])) {
	$mapping['joinTable']['joinColumns'] = [
	[
	'name' => $this->namingStrategy->joinKeyColumnName($mapping['sourceEntity'], $selfReferencingEntityWithoutJoinColumns ? 'source' : null),
	'referencedColumnName' => $this->namingStrategy->referenceColumnName(),
	'onDelete' => 'CASCADE',
	],
	];
	}
	if (! isset($mapping['joinTable']['inverseJoinColumns'])) {
	$mapping['joinTable']['inverseJoinColumns'] = [
	[
	'name' => $this->namingStrategy->joinKeyColumnName($mapping['targetEntity'], $selfReferencingEntityWithoutJoinColumns ? 'target' : null),
	'referencedColumnName' => $this->namingStrategy->referenceColumnName(),
	'onDelete' => 'CASCADE',
	],
	];
	}
	$mapping['joinTableColumns'] = [];
	foreach ($mapping['joinTable']['joinColumns'] as &$joinColumn) {
	if (empty($joinColumn['name'])) {
	$joinColumn['name'] = $this->namingStrategy->joinKeyColumnName($mapping['sourceEntity'], $joinColumn['referencedColumnName']);
	}
	if (empty($joinColumn['referencedColumnName'])) {
	$joinColumn['referencedColumnName'] = $this->namingStrategy->referenceColumnName();
	}
	if ($joinColumn['name'][0] === '`') {
	$joinColumn['name'] = trim($joinColumn['name'], '`');
	$joinColumn['quoted'] = true;
	}
	if ($joinColumn['referencedColumnName'][0] === '`') {
	$joinColumn['referencedColumnName'] = trim($joinColumn['referencedColumnName'], '`');
	$joinColumn['quoted'] = true;
	}
	if (isset($joinColumn['onDelete']) && strtolower($joinColumn['onDelete']) === 'cascade') {
	$mapping['isOnDeleteCascade'] = true;
	}
	$mapping['relationToSourceKeyColumns'][$joinColumn['name']] = $joinColumn['referencedColumnName'];
	$mapping['joinTableColumns'][] = $joinColumn['name'];
	}
	foreach ($mapping['joinTable']['inverseJoinColumns'] as &$inverseJoinColumn) {
	if (empty($inverseJoinColumn['name'])) {
	$inverseJoinColumn['name'] = $this->namingStrategy->joinKeyColumnName($mapping['targetEntity'], $inverseJoinColumn['referencedColumnName']);
	}
	if (empty($inverseJoinColumn['referencedColumnName'])) {
	$inverseJoinColumn['referencedColumnName'] = $this->namingStrategy->referenceColumnName();
	}
	if ($inverseJoinColumn['name'][0] === '`') {
	$inverseJoinColumn['name'] = trim($inverseJoinColumn['name'], '`');
	$inverseJoinColumn['quoted'] = true;
	}
	if ($inverseJoinColumn['referencedColumnName'][0] === '`') {
	$inverseJoinColumn['referencedColumnName'] = trim($inverseJoinColumn['referencedColumnName'], '`');
	$inverseJoinColumn['quoted'] = true;
	}
	if (isset($inverseJoinColumn['onDelete']) && strtolower($inverseJoinColumn['onDelete']) === 'cascade') {
	$mapping['isOnDeleteCascade'] = true;
	}
	$mapping['relationToTargetKeyColumns'][$inverseJoinColumn['name']] = $inverseJoinColumn['referencedColumnName'];
	$mapping['joinTableColumns'][] = $inverseJoinColumn['name'];
	}
	}

[mpdude]

Then let's see that we define the class hierarchy not only by technical criteria (i.e. which fields are common in several classes and extract those into a base class), but from a semantical point of view. What kind of checks do we have in the codebase?

For example if ($mapping['isOwningSide']) ... would become if ($mapping instanceof AssociationOwningSide) ... or so.

[greg0ire]

I think we have two kind of checks: you just mentioned the first one, and the other one is against $mapping['type'], sometimes with the & operator, for instance to detect if it's a to-many, for instance here:

orm/lib/Doctrine/ORM/PersistentCollection.php

Line 354 in 31ff969

$this->association['type'] & ClassMetadata::TO_MANY &&

We might need to introduce ToOneAssociationMapping and ToManyAssociationMapping interfaces. ATM I used those names for traits but I can change those to To{One,Many}AssociationMappingTrait I guess.

The thing is, interfaces can't have properties, so it's only a viable option if we switch to methods instead.

[greg0ire]

Ah but we only ever test for TO_MANY and TO_ONE, never for ONE_TO or MANY_TO (those constants don't even exist), so it should be possible to just include these as classes in the hierarchy. I will ditch the traits and do that instead.

[greg0ire]

I pushed a new commit on the other PR, switching from traits to classes.

The thing is, interfaces can't have properties, so it's only a viable option if we switch to methods instead.

Just realised that this might not be an issue because we should be able to change the phpdoc on ClassMetadata::AssociationMapping from array<string, AssociationMapping> to array<string, list-of-concrete-classes>. An instanceof check should allow SA to rule out some of the classes and to notice that some of them have common properties 🤞