From a7747eadd5cf80b2d824a60de391cf2b4fe694a1 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 20 Aug 2026 14:04:28 +0200 Subject: [PATCH 01/12] BelongsToMany: Accept `NULL` as target foreign/candidate key This is required to establish type symmetry as the base's methods also accept `NULL` to be able to direcly pass a getters return value to the appropriate setter. --- src/Relation/BelongsToMany.php | 8 ++++---- tests/BelongsToManyTest.php | 10 ++++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/Relation/BelongsToMany.php b/src/Relation/BelongsToMany.php index bf570f63..504cfbaa 100644 --- a/src/Relation/BelongsToMany.php +++ b/src/Relation/BelongsToMany.php @@ -142,11 +142,11 @@ public function getTargetForeignKey(): string|array|null /** * Set the column name(s) of the target model's foreign key found in the join table * - * @param string|array $targetForeignKey Array if the foreign key is compound, string otherwise + * @param string|array|null $targetForeignKey Array if the foreign key is compound, string otherwise * * @return $this */ - public function setTargetForeignKey(string|array $targetForeignKey): static + public function setTargetForeignKey(string|array|null $targetForeignKey): static { $this->targetForeignKey = $targetForeignKey; @@ -166,11 +166,11 @@ public function getTargetCandidateKey(): string|array|null /** * Set the candidate key column name(s) in the target table which references the target foreign key * - * @param string|array $targetCandidateKey Array if the foreign key is compound, string otherwise + * @param string|array|null $targetCandidateKey Array if the foreign key is compound, string otherwise * * @return $this */ - public function setTargetCandidateKey(string|array $targetCandidateKey): static + public function setTargetCandidateKey(string|array|null $targetCandidateKey): static { $this->targetCandidateKey = $targetCandidateKey; diff --git a/tests/BelongsToManyTest.php b/tests/BelongsToManyTest.php index cf37846a..8e4b6056 100644 --- a/tests/BelongsToManyTest.php +++ b/tests/BelongsToManyTest.php @@ -174,4 +174,14 @@ public function testResolveYieldsJunctionAndTargetRelationsWithTheirFiltersAndJo 'The target join does not carry the relation filter' ); } + + public function testSetTargetForeignKeyAcceptsNull() + { + $this->assertNull((new BelongsToMany())->setTargetForeignKey(null)->getTargetForeignKey()); + } + + public function testSetTargetCandidateKeyAcceptsNull() + { + $this->assertNull((new BelongsToMany())->setTargetCandidateKey(null)->getTargetCandidateKey()); + } } From 3eb984d58f6fd3df0afb7770b8831281e7dcfdd9 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 20 Aug 2026 14:17:09 +0200 Subject: [PATCH 02/12] Resolver: Leave it up to a relation to resolve it There is now `Relation::bindTo(Model, string, Resolver)` in order to pass control to relations how they're prepared. Since the introduction of `BelongsToMany`, it is established that a relation may resolve to multiple hops and thus needs to perform steps n-times rather than a single time. It's this reason because registering the alias and resolving the filter is now a responsibility of a relation rather than the resolver. Relations know it better how to and the override of `bindTo` in `BelongsToMany` proves it as it turned out that it is necessary to allow referencing the junction table in either the filter or the through filter in order to be able to better reverse relations. Qualification must be done by a relation in turn as well, as otherwise there's a mis-match with what's allowed to reference and what can be qualified. My initial attempt was to teach `Relation::resolve()` this, but without passing it the resolver and changing the return value this doesn't make sense. Sadly, this is out of the question as this is a breaking change. Say hello to `Relation::setFilterSubjects()` due to this. --- src/Query.php | 17 ++++-- src/Relation.php | 62 +++++++++++++++++++- src/Relation/BelongsToMany.php | 61 ++++++++++++++++++++ src/Resolver.php | 100 +++++++++------------------------ tests/BelongsToManyTest.php | 46 ++++++++------- tests/Lib/Model/Department.php | 2 +- tests/ResolverTest.php | 45 +++++++++++---- 7 files changed, 224 insertions(+), 109 deletions(-) diff --git a/src/Query.php b/src/Query.php index e1b47f31..745e1795 100644 --- a/src/Query.php +++ b/src/Query.php @@ -333,7 +333,7 @@ public function getSelectBase(): Select $visibilityFilter = FilterProcessor::assembleFilter( $this->getResolver()->qualifyFilter( $this->getResolver()->getVisibilityFilter($this->getModel()), - $this->getModel() + ...[$this->getModel()->getTableAlias() => $this->getModel()] ) ); if ($visibilityFilter) { @@ -516,15 +516,21 @@ public function assembleSelect(): Select foreach ($relation->resolve() as $targetRelation => [$source, $target, $relatedKeys]) { if (is_int($targetRelation)) { $targetRelation = $relation; + $relationFilter = Filter::any(); trigger_error(sprintf( 'Relation implementation of %s::resolve() returned a numeric key for the target' . ' relation. This is deprecated and will be removed in a future version. Please return' . ' the target relation as key instead.', $relation::class ), E_USER_DEPRECATED); + } else { + /** @var Relation $targetRelation */ + $relationFilter = $resolver->qualifyFilter( + $targetRelation->getFilter(), + ...$targetRelation->getFilterSubjects() + ); } - /** @var Relation $targetRelation */ /** @var Model $source */ /** @var Model $target */ @@ -541,8 +547,11 @@ public function assembleSelect(): Select } $visibilityConditions = FilterProcessor::assembleFilter(Filter::all( - $resolver->qualifyFilter($targetRelation->getFilter(), $targetRelation), - $resolver->qualifyFilter($resolver->getVisibilityFilter($target), $target) + $relationFilter, + $resolver->qualifyFilter( + $resolver->getVisibilityFilter($target), + ...[$target->getTableAlias() => $target] + ) )); if ($visibilityConditions) { $conditions[] = $visibilityConditions; diff --git a/src/Relation.php b/src/Relation.php index d9a9d9f8..9b775196 100644 --- a/src/Relation.php +++ b/src/Relation.php @@ -5,6 +5,7 @@ use Generator; use ipl\Stdlib\Filter; use ipl\Stdlib\Filter\Rule; +use LogicException; use UnexpectedValueException; /** @@ -43,6 +44,8 @@ class Relation /** @var ?Filter\Chain Additional JOIN conditions */ protected ?Filter\Chain $filter = null; + /** @var ?array Models additional JOIN conditions may reference, keyed by their alias */ + protected ?array $filterSubjects = null; /** * Get the default column name(s) in the source table used to match the foreign key * @@ -298,6 +301,34 @@ public function setFilter(Filter\Rule $filter): static return $this; } + /** + * Get subjects the relation filter may reference + * + * @return array + */ + public function getFilterSubjects(): array + { + return $this->filterSubjects ?? throw new LogicException(sprintf( + 'Cannot get filter subjects of an unbound relation. Please call %s::bindTo() first.', + static::class + )); + } + + /** + * Add subjects the relation filter may reference, while keeping existing ones + * + * @param array ...$subjects + * + * @return $this + */ + public function addFilterSubjects(Model ...$subjects): static + { + $this->filterSubjects ??= []; + $this->filterSubjects += $subjects; + + return $this; + } + /** * Determine the candidate key-foreign key construct of the relation * @@ -348,13 +379,42 @@ public function determineKeys(Model $source): array return array_combine($foreignKey, $candidateKey); } + /** + * Bind the relation to the given source using the passed resolver + * + * @param Model $source The model to use as source + * @param string $path The path the relation has been resolved at + * @param Resolver $resolver The resolver to register the relation's target alias + * + * @return $this + */ + public function bindTo(Model $source, string $path, Resolver $resolver): static + { + $this->setSource($source); + $target = $this->getTarget(); + + $subjects = [ + $this->getName() => $target, + $target->getTableAlias() => $target, + $source->getTableAlias() => $source + ]; + + $this->addFilterSubjects(...$subjects); + + $resolver->resolveRelationFilter($this->getFilter(), $this->getName(), ...$subjects); + $resolver->setAlias($target, str_replace('.', '_', $path)); + + return $this; + } + /** * Resolve the relation * * Yields the relation to join as key and a three-element array consisting of the source model, * target model and the join keys as value. * - * @return Generator}, void> + * @return Generator}, void> + * @phpstan-return Generator}, mixed, void> */ public function resolve(): Generator { diff --git a/src/Relation/BelongsToMany.php b/src/Relation/BelongsToMany.php index 504cfbaa..2bda2e96 100644 --- a/src/Relation/BelongsToMany.php +++ b/src/Relation/BelongsToMany.php @@ -6,6 +6,7 @@ use ipl\Orm\Model; use ipl\Orm\Relation; use ipl\Orm\Relations; +use ipl\Orm\Resolver; use ipl\Stdlib\Filter; use ipl\Stdlib\Filter\Rule; use LogicException; @@ -38,6 +39,9 @@ class BelongsToMany extends Relation /** @var ?Filter\Chain Additional JOIN conditions for the join table */ protected ?Filter\Chain $throughFilter = null; + /** @var ?array Models additional join table conditions may reference, keyed by their alias */ + protected ?array $throughFilterSubjects = null; + /** * Get the name of the join table or junction model class * @@ -211,6 +215,61 @@ public function setThroughFilter(Filter\Rule $filter): static return $this; } + /** + * Get subjects the join table filter may reference + * + * @return array + */ + public function getThroughFilterSubjects(): array + { + return $this->throughFilterSubjects ?? throw new LogicException(sprintf( + 'Cannot get filter subjects of an unbound relation. Please call %s::bindTo() first.', + static::class + )); + } + + /** + * Add subjects the join table filter may reference, while keeping existing ones + * + * @param array ...$subjects + * + * @return $this + */ + public function addThroughFilterSubjects(Model ...$subjects): static + { + $this->throughFilterSubjects ??= []; + $this->throughFilterSubjects += $subjects; + + return $this; + } + + public function bindTo(Model $source, string $path, Resolver $resolver): static + { + // Allow to reference the join table in the second hop + $this->addFilterSubjects(...[$this->getThroughAlias() => $this->getThrough()]); + + parent::bindTo($source, $path, $resolver); + + $this->addThroughFilterSubjects(...[ + $this->getSource()->getTableAlias() => $this->getSource(), + $this->getThrough()->getTableAlias() => $this->getThrough(), + $this->getThroughAlias() => $this->getThrough() + ]); + + $resolver->resolveRelationFilter( + $this->getThroughFilter(), + $this->getThroughAlias(), + ...$this->getThroughFilterSubjects() + ); + + $resolver->setAlias($this->getThrough(), join('_', array_merge( + array_slice(explode('.', $path), 0, -1), + [$this->getThroughAlias()] + ))); + + return $this; + } + public function resolve(): Generator { $source = $this->getSource(); @@ -250,6 +309,7 @@ public function resolve(): Generator ->setSource($source) ->setTarget($junction) ->setFilter($this->getThroughFilter()) + ->addFilterSubjects(...$this->getThroughFilterSubjects()) ->setCandidateKey($this->extractKey($possibleCandidateKey)) ->setForeignKey($this->extractKey($possibleForeignKey)) ->setJoinType($this->getJoinType()); @@ -262,6 +322,7 @@ public function resolve(): Generator ->setSource($junction) ->setTarget($target) ->setFilter($this->getFilter()) + ->addFilterSubjects(...$this->getFilterSubjects()) ->setCandidateKey($this->extractKey($possibleTargetCandidateKey)) ->setForeignKey($this->extractKey($possibleTargetForeignKey)) ->setJoinType($this->getJoinType()); diff --git a/src/Resolver.php b/src/Resolver.php index 68cf0902..fb2fb862 100644 --- a/src/Resolver.php +++ b/src/Resolver.php @@ -500,37 +500,32 @@ public function qualifyPath(string $path, string $tableName): string /** * Resolve the given relation filter * - * Resolves each condition's column according to the referenced subject or, by default, the target. - * The target may also be referenced by the relation's name. + * Resolves each condition's column according to the referenced models or to the given default. * * @param Filter\Chain $filter - * @param string $name The name of the relation - * @param Model $source - * @param Model $target + * @param string $default Must be a valid subject + * @param array $subjects Models keyed by their name * * @throws InvalidArgumentException If a non-condition rule or invalid column is used in the filter */ - public function resolveRelationFilter(Filter\Chain $filter, string $name, Model $source, Model $target): void + public function resolveRelationFilter(Filter\Chain $filter, string $default, Model ...$subjects): void { - $resolveColumn = function (string $column) use ($name, $source, $target): string { + $resolveColumn = function (string $column) use ($default, $subjects): string { // A column may reference the source or target table by its alias, defaulting to the target if (str_contains($column, '.')) { [$alias, $column] = explode('.', $column, 2); } else { - $alias = $target->getTableAlias(); + $alias = $default; } - $subject = match ($alias) { - $name => $target, - $source->getTableAlias() => $source, - $target->getTableAlias() => $target, - default => throw new InvalidArgumentException(sprintf( - 'Invalid relation alias "%s" for models "%s" and "%s"', - $alias, - get_class($source), - get_class($target) + $subject = $subjects[$alias] ?? throw new InvalidArgumentException(sprintf( + 'Invalid relation alias "%s". Available options are: %s', + $alias, + join(', ', array_map( + fn($k) => sprintf('%s => %s', $k, get_class($subjects[$k])), + array_keys($subjects) )) - }; + )); if (! $subject instanceof Junction && ! $this->hasSelectableColumn($subject, $column)) { throw new InvalidArgumentException(sprintf( @@ -546,8 +541,7 @@ public function resolveRelationFilter(Filter\Chain $filter, string $name, Model foreach ($filter->yieldRules() as $rule) { if (! $rule instanceof Filter\Condition) { throw new InvalidArgumentException(sprintf( - 'Relation filter for model "%s" contains a non-condition rule of type "%s"', - get_class($target), + 'Relation filter contains a non-condition rule of type "%s"', get_class($rule) )); } @@ -566,42 +560,24 @@ public function resolveRelationFilter(Filter\Chain $filter, string $name, Model * Qualify the columns of the given filter * * @param Filter\Chain $filter - * @param Model|Relation $subject + * @param array $subjects Models keyed by their name * * @return Filter\Chain * * @throws InvalidArgumentException If a non-condition rule is used or an unknown model is referenced */ - public function qualifyFilter(Filter\Chain $filter, Model|Relation $subject): Filter\Chain + public function qualifyFilter(Filter\Chain $filter, Model ...$subjects): Filter\Chain { - $qualifyColumn = function (string $column) use ($subject): string { + $qualifyColumn = function (string $column) use ($subjects): string { [$alias, $column] = explode('.', $column, 2); - if ($subject instanceof Model) { - if ($subject->getTableAlias() !== $alias) { - throw new InvalidArgumentException(sprintf( - 'Unknown model alias "%s" for filter column "%s"', - $alias, - $column - )); - } - - return $this->qualifyColumn($column, $this->getAlias($subject)); - } + $subject = $subjects[$alias] ?? throw new InvalidArgumentException(sprintf( + 'Unknown model alias "%s" for filter column "%s"', + $alias, + $column + )); - return $this->qualifyColumn( - $column, - match ($alias) { - $subject->getSource()->getTableAlias() => $this->getAlias($subject->getSource()), - $subject->getTarget()->getTableAlias() => $this->getAlias($subject->getTarget()), - $subject->getName() => $this->getAlias($subject->getTarget()), - default => throw new InvalidArgumentException(sprintf( - 'Unknown model alias "%s" for filter column "%s"', - $alias, - $column - )) - } - ); + return $this->qualifyColumn($column, $this->getAlias($subject)); }; $filter = clone $filter; // Deep clone @@ -670,7 +646,9 @@ public function resolveRelation(string $path, ?Model $subject = null): Relation * @param string $path * @param ?Model $subject * - * @return Generator + * @return Generator + * @phpstan-return Generator + * * @throws InvalidArgumentException In case $path is not fully qualified * @throws InvalidRelationException In case a relation is unknown */ @@ -715,32 +693,10 @@ public function resolveRelations(string $path, ?Model $subject = null): Generato throw new InvalidRelationException($relationName, $target); } - $relation = $targetRelations->get($relationName); - $relation->setSource($target); - $this->resolveRelationFilter( - $relation->getFilter(), - $relationName, - $target, - $relation->getTarget() - ); + $relation = $targetRelations->get($relationName) + ->bindTo($target, $relationPath, $this); $resolvedRelations[$relationPath] = $relation; - - if ($relation instanceof BelongsToMany) { - $this->resolveRelationFilter( - $relation->getThroughFilter(), - $relationName, - $target, - $relation->getThrough() - ); - - $this->setAlias($relation->getThrough(), join('_', array_merge( - array_slice($segments, 0, -1), - [$relation->getThroughAlias()] - ))); - } - - $this->setAlias($relation->getTarget(), join('_', $segments)); } yield $relationPath => $relation; diff --git a/tests/BelongsToManyTest.php b/tests/BelongsToManyTest.php index 8e4b6056..f1fbf468 100644 --- a/tests/BelongsToManyTest.php +++ b/tests/BelongsToManyTest.php @@ -5,6 +5,7 @@ use ipl\Orm\Query; use ipl\Orm\Relation\BelongsToMany; use ipl\Orm\Relations; +use ipl\Orm\Resolver; use ipl\Sql\Test\SqlAssertions; use ipl\Stdlib\Filter; @@ -40,7 +41,7 @@ public function testResolveDefaultKeys() foreach ( $relations ->get('user') - ->setSource($model) + ->bindTo($model, 'car.user', $this->createStub(Resolver::class)) ->resolve() as [$from, $to, $keys] ) { reset($keys); @@ -77,7 +78,7 @@ public function testResolveRespectsCustomKeysInTroughModels() foreach ( $relations ->get('user_custom_keys') - ->setSource($model) + ->bindTo($model, 'car.user_custom_keys', $this->createStub(Resolver::class)) ->resolve() as [$from, $to, $keys] ) { reset($keys); @@ -133,22 +134,17 @@ public function testSetThroughFilterWrapsABareConditionInAnAllChain() public function testResolveYieldsJunctionAndTargetRelationsWithTheirFiltersAndJoinType() { - $model = new Car(); - $relations = new Relations(); - $model->createRelations($relations); + $query = (new Query())->setModel(new Car()); + $resolver = $query->getResolver(); - $throughFilter = Filter::equal('user_id', 5); - $targetFilter = Filter::equal('username', 'root'); - - $relation = $relations - ->get('user') - ->setSource($model) + $resolver->getRelations($query->getModel())->get('user') ->setJoinType('LEFT') - ->setThroughFilter($throughFilter) - ->setFilter($targetFilter); + ->setThroughFilter(Filter::equal('user_id', 5)) + ->setFilter(Filter::equal('username', 'root')) + ->bindTo($query->getModel(), 'car.user', $resolver); $resolved = []; - foreach ($relation->resolve() as $key => $_) { + foreach ($resolver->resolveRelation('car.user')->resolve() as $key => $_) { $resolved[] = $key; } @@ -160,18 +156,28 @@ public function testResolveYieldsJunctionAndTargetRelationsWithTheirFiltersAndJo $this->assertSame('LEFT', $toJunction->getJoinType()); $this->assertSame('LEFT', $toTarget->getJoinType()); + // The junction sits between source and target + $this->assertSame('car', $toJunction->getSource()->getTableName()); + $this->assertSame('car_user', $toJunction->getTarget()->getTableName()); + $this->assertSame('car_user', $toTarget->getSource()->getTableName()); + $this->assertSame('user', $toTarget->getTarget()->getTableName()); + // The junction join carries the through filter ... + $throughFilter = iterator_to_array($toJunction->getFilter()->yieldRules()); + $this->assertNotEmpty($throughFilter, 'The junction join does not carry the through filter'); $this->assertSame( - [$throughFilter], - iterator_to_array($toJunction->getFilter()), - 'The junction join does not carry the through filter' + 'car_user.user_id', + $throughFilter[0]->getColumn(), + 'The through filter column is incorrectly resolved' ); // ... and the target join carries the relation filter + $relationFilter = iterator_to_array($toTarget->getFilter()->yieldRules()); + $this->assertNotEmpty($relationFilter, 'The target join does not carry the relation filter'); $this->assertSame( - [$targetFilter], - iterator_to_array($toTarget->getFilter()), - 'The target join does not carry the relation filter' + 'user.username', + $relationFilter[0]->getColumn(), + 'The relation filter column is incorrectly resolved' ); } diff --git a/tests/Lib/Model/Department.php b/tests/Lib/Model/Department.php index 1c4cdfd7..ce72ef13 100644 --- a/tests/Lib/Model/Department.php +++ b/tests/Lib/Model/Department.php @@ -33,7 +33,7 @@ public function createRelations(Relations $relations) // Relation filter referencing the target (default) and the source table alias $relations->hasMany('lead', Employee::class) ->setFilter(Filter::all( - Filter::equal('role', 'lead'), + Filter::equal('employee.role', 'lead'), Filter::equal('department.name', 'Engineering') )); } diff --git a/tests/ResolverTest.php b/tests/ResolverTest.php index 633cdc6a..a47cd8f7 100644 --- a/tests/ResolverTest.php +++ b/tests/ResolverTest.php @@ -226,10 +226,14 @@ public function testResolveRelationFilterQualifiesTargetColumnsByDefault() $resolver = (new Query())->setModel(new Department())->getResolver(); $filter = Filter::all(Filter::equal('active', 'y'), Filter::equal('employee.role', 'lead')); - $resolver->resolveRelationFilter($filter, 'relation', new Department(), new Employee()); + $resolver->resolveRelationFilter($filter, 'relation', ...[ + 'relation' => new Employee(), + 'employee' => new Employee(), + 'department' => new Department() + ]); $columns = array_map(fn ($rule) => $rule->getColumn(), iterator_to_array($filter->yieldRules())); - $this->assertSame(['employee.active', 'employee.role'], $columns); + $this->assertSame(['relation.active', 'employee.role'], $columns); } public function testResolveRelationFilterQualifiesSourceColumns() @@ -237,7 +241,11 @@ public function testResolveRelationFilterQualifiesSourceColumns() $resolver = (new Query())->setModel(new Department())->getResolver(); $filter = Filter::all(Filter::equal('department.name', 'Engineering')); - $resolver->resolveRelationFilter($filter, 'relation', new Department(), new Employee()); + $resolver->resolveRelationFilter($filter, 'relation', ...[ + 'relation' => new Employee(), + 'employee' => new Employee(), + 'department' => new Department() + ]); $this->assertSame('department.name', iterator_to_array($filter->yieldRules())[0]->getColumn()); } @@ -247,7 +255,10 @@ public function testResolveRelationFilterQualifiesRelationColumns() $resolver = (new Query())->setModel(new Department())->getResolver(); $filter = Filter::all(Filter::equal('supplementary.name', 'Q/A')); - $resolver->resolveRelationFilter($filter, 'supplementary', new Department(), new Department()); + $resolver->resolveRelationFilter($filter, 'supplementary', ...[ + 'supplementary' => new Department(), + 'department' => new Department() + ]); $this->assertSame('supplementary.name', iterator_to_array($filter->yieldRules())[0]->getColumn()); } @@ -262,8 +273,11 @@ public function testResolveRelationFilterThrowsForAnUnknownAlias() $resolver->resolveRelationFilter( Filter::all(Filter::equal('office.city', 'London')), 'relation', - new Department(), - new Employee() + ...[ + 'relation' => new Employee(), + 'employee' => new Employee(), + 'department' => new Department() + ] ); } @@ -277,8 +291,11 @@ public function testResolveRelationFilterThrowsForANonSelectableColumn() $resolver->resolveRelationFilter( Filter::all(Filter::equal('unknown', 'x')), 'relation', - new Department(), - new Employee() + ...[ + 'relation' => new Employee(), + 'employee' => new Employee(), + 'department' => new Department() + ] ); } @@ -288,7 +305,11 @@ public function testResolveRelationFilterDoesNotValidateJunctionColumns() $junction = (new Junction())->setTableName('membership'); $filter = Filter::all(Filter::equal('membership.since', '2020')); - $resolver->resolveRelationFilter($filter, 'relation', new Department(), $junction); + $resolver->resolveRelationFilter($filter, 'relation', ...[ + 'relation' => $junction, + 'membership' => $junction, + 'department' => new Department() + ]); $this->assertSame('membership.since', iterator_to_array($filter->yieldRules())[0]->getColumn()); } @@ -302,7 +323,7 @@ public function testQualifyFilterThrowsForAnUnknownModelAlias() $query->getResolver()->qualifyFilter( Filter::all(Filter::equal('employee.active', 'y')), - $query->getModel() + ...[$query->getModel()->getTableAlias() => $query->getModel()] ); } @@ -311,7 +332,9 @@ public function testQualifyFilterDoesNotModifyTheGivenFilter() $query = (new Query())->setModel(new Department()); $original = Filter::all(Filter::equal('department.name', 'Engineering')); - $qualified = $query->getResolver()->qualifyFilter($original, $query->getModel()); + $qualified = $query->getResolver()->qualifyFilter($original, ...[ + $query->getModel()->getTableAlias() => $query->getModel() + ]); // The chain is deep cloned, hence the original is left untouched $this->assertNotSame($original, $qualified, 'The given filter has not been cloned'); From f0282f0b648e115381ab53d6b4bacbf305aa6ea7 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 20 Aug 2026 14:21:47 +0200 Subject: [PATCH 03/12] Relation: Add method `reverse(Resolver): Generator` Changes the way relations can be reversed drastically as it is now possible to influence the relation to use during reversal with `::setReverseName(string)` which allows Icinga DB Web to drop the error-prone `to.from` and `from.to` relations. An additional change is that it is now not mandatory anymore to define relations that are solely being required because of sub-queries. Missing relations on the reversed path are automatically registered. For this, each relation type now has its specific counterpart which is possible to override with `::setReverseClass(class-string)`. The default however, is to use the same type which is the case for `BelongsToOne` and `BelongsToMany`. For `BelongsTo` a sane override has been chosen that is based on how it's used at the moment in our products, as `HasOne` and `HasMany` may both be appropriate. But the latter clearly is used more often. --- src/Relation.php | 138 +++++++++++++++++++++++++++++++++ src/Relation/BelongsTo.php | 2 + src/Relation/BelongsToMany.php | 36 +++++++++ src/Relation/HasMany.php | 2 + src/Relation/HasOne.php | 1 + tests/BelongsToManyTest.php | 29 +++++++ tests/Lib/Model/Author.php | 33 ++++++++ tests/Lib/Model/Book.php | 40 ++++++++++ tests/RelationTest.php | 103 ++++++++++++++++++++++++ 9 files changed, 384 insertions(+) create mode 100644 tests/Lib/Model/Author.php create mode 100644 tests/Lib/Model/Book.php diff --git a/src/Relation.php b/src/Relation.php index 9b775196..578463ca 100644 --- a/src/Relation.php +++ b/src/Relation.php @@ -6,6 +6,7 @@ use ipl\Stdlib\Filter; use ipl\Stdlib\Filter\Rule; use LogicException; +use RuntimeException; use UnexpectedValueException; /** @@ -17,6 +18,12 @@ class Relation /** @var string Name of the relation */ protected $name; + /** @var ?string Name of the reversed relation */ + protected ?string $reverseName = null; + + /** @var ?class-string The class to reverse the relation */ + protected ?string $reverseClass = null; + /** @var Model Source model */ protected $source; @@ -46,6 +53,10 @@ class Relation /** @var ?array Models additional JOIN conditions may reference, keyed by their alias */ protected ?array $filterSubjects = null; + + /** @var ?string The name of the relation prior reversal */ + private ?string $forwardRelationName = null; + /** * Get the default column name(s) in the source table used to match the foreign key * @@ -115,6 +126,56 @@ public function setName(string $name): static return $this; } + /** + * Get the reverse name of the relation + * + * @return ?string + */ + public function getReverseName(): ?string + { + return $this->reverseName; + } + + /** + * Set the reverse name of the relation + * + * The source's table alias is used by default. + * + * @param string $name + * + * @return $this + */ + public function setReverseName(string $name): static + { + $this->reverseName = $name; + + return $this; + } + + /** + * Get the class to reverse the relation + * + * @return class-string + */ + public function getReverseClass(): string + { + return $this->reverseClass ?? static::class; + } + + /** + * Set the class to reverse the relation + * + * @param class-string $reverseClass + * + * @return $this + */ + public function setReverseClass(string $reverseClass): static + { + $this->reverseClass = $reverseClass; + + return $this; + } + /** * Get the source model of the relation * @@ -398,6 +459,9 @@ public function bindTo(Model $source, string $path, Resolver $resolver): static $target->getTableAlias() => $target, $source->getTableAlias() => $source ]; + if ($this->forwardRelationName !== null) { + $subjects[$this->forwardRelationName] = $source; + } $this->addFilterSubjects(...$subjects); @@ -422,4 +486,78 @@ public function resolve(): Generator yield $this => [$source, $this->getTarget(), $this->determineKeys($source)]; } + + /** + * Reverse the relation + * + * Uses the passed resolver to eagerly register missing relations on the reversed path. + * + * @param Resolver $resolver + * + * @return Generator + * @phpstan-return Generator + * + * @throws LogicException In case the relation is not bound yet (has no source) or has already been reversed + * @throws RuntimeException In case the model of the forward relation is incompatible with the reversed relation's + */ + public function reverse(Resolver $resolver): Generator + { + if ($this->getSource() === null) { + throw new LogicException('Cannot reverse an unbound relation.'); + } elseif (isset($this->forwardRelationName)) { + throw new LogicException('Cannot undo a reverse.'); + } + + $reverseName = $this->getReverseName() ?? $this->getSource()->getTableAlias(); + + $targetRelations = $resolver->getRelations($this->getTarget()); + if ($targetRelations->has($reverseName)) { + // Explicit reverse relations must be properly set up with corresponding key pairs + $relation = $targetRelations->get($reverseName); + + if (! $this->getSource() instanceof ($relation->getTargetClass())) { + throw new RuntimeException(sprintf( + 'The source model of the relation "%s" (%s) is not compatible' + . ' with the target model of the inverse relation (%s)', + $this->getName(), + get_class($this->getSource()), + $relation->getTargetClass() + )); + } + } else { + // Eagerly create the relation in case it's only necessary during reversal + $relation = $targetRelations->create( + $this->getReverseClass(), + $reverseName, + get_class($this->getSource()) + ); + + // Pass on custom configuration + $relation->setCandidateKey($this->getForeignKey()); + $relation->setForeignKey($this->getCandidateKey()); + $relation->setJoinType($this->getJoinType()); + } + + // The previous relation name must be kept for reference as relation filters + // may require it but need to be resolved to the source model instead. + $relation->forwardRelationName = $this->getName(); + + $relation->setTarget($this->getSource()); // Propagates the same instance + + if (! $this->getFilter()->isEmpty()) { + // Do not override set filters with an empty set, however, if the set is not empty + // the forward relation is expected to carry the same semantics as the inverse. + $relation->setFilter(clone $this->getFilter()); + } + + yield $relation; + + if (! $targetRelations->has($relation->getName())) { + /** + * This is done after `yield` so that the backwards compatibility branch + * of {@see Query::createSubQuery()} is able to change the name. + */ + $targetRelations->add($relation); + } + } } diff --git a/src/Relation/BelongsTo.php b/src/Relation/BelongsTo.php index 1982f10b..61419fce 100644 --- a/src/Relation/BelongsTo.php +++ b/src/Relation/BelongsTo.php @@ -10,4 +10,6 @@ class BelongsTo extends Relation { protected bool $inverse = true; + + protected ?string $reverseClass = HasMany::class; } diff --git a/src/Relation/BelongsToMany.php b/src/Relation/BelongsToMany.php index 2bda2e96..9aa161a6 100644 --- a/src/Relation/BelongsToMany.php +++ b/src/Relation/BelongsToMany.php @@ -10,6 +10,7 @@ use ipl\Stdlib\Filter; use ipl\Stdlib\Filter\Rule; use LogicException; +use RuntimeException; /** * Many-to-many relationship @@ -330,6 +331,41 @@ public function resolve(): Generator yield from $toTarget->resolve(); } + public function reverse(Resolver $resolver): Generator + { + foreach (parent::reverse($resolver) as $relation) { + if ($relation->getThroughClass() !== null && $relation->getThroughClass() !== $this->getThroughClass()) { + throw new RuntimeException(sprintf( + 'The junction model of the relation "%s" (%s) is not compatible' + . ' with the junction model of the inverse relation (%s != %s)', + $this->getName(), + get_class($relation->getSource()), + $relation->getThroughClass(), + $this->getThroughClass() + )); + } + + $relation->through($this->getThroughClass()); + $relation->setThrough($this->getThrough()); + $relation->setThroughAlias($this->getThroughAlias()); + + if (! $this->getThroughFilter()->isEmpty()) { + $relation->setThroughFilter(clone $this->getThroughFilter()); + } + + yield $relation; + + if (! $resolver->getRelations($this->getTarget())->has($relation->getName())) { + // The relation is eagerly set up and thus needs proper key pairs, + // but reversed as only the forward relation's pairs are known. + $relation->setCandidateKey($this->getTargetCandidateKey()); + $relation->setForeignKey($this->getTargetForeignKey()); + $relation->setTargetCandidateKey($this->getCandidateKey()); + $relation->setTargetForeignKey($this->getForeignKey()); + } + } + } + protected function extractKey(array $possibleKey): string|array|null { $filtered = array_filter($possibleKey); diff --git a/src/Relation/HasMany.php b/src/Relation/HasMany.php index 13d1abc4..82ca669e 100644 --- a/src/Relation/HasMany.php +++ b/src/Relation/HasMany.php @@ -10,4 +10,6 @@ class HasMany extends Relation { protected bool $isOne = false; + + protected ?string $reverseClass = BelongsTo::class; } diff --git a/src/Relation/HasOne.php b/src/Relation/HasOne.php index 8f7a802a..fe56aa4b 100644 --- a/src/Relation/HasOne.php +++ b/src/Relation/HasOne.php @@ -9,4 +9,5 @@ */ class HasOne extends Relation { + protected ?string $reverseClass = BelongsTo::class; } diff --git a/tests/BelongsToManyTest.php b/tests/BelongsToManyTest.php index f1fbf468..0e8a4934 100644 --- a/tests/BelongsToManyTest.php +++ b/tests/BelongsToManyTest.php @@ -8,6 +8,7 @@ use ipl\Orm\Resolver; use ipl\Sql\Test\SqlAssertions; use ipl\Stdlib\Filter; +use ipl\Tests\Orm\Lib\Model\Book; class BelongsToManyTest extends \PHPUnit\Framework\TestCase { @@ -190,4 +191,32 @@ public function testSetTargetCandidateKeyAcceptsNull() { $this->assertNull((new BelongsToMany())->setTargetCandidateKey(null)->getTargetCandidateKey()); } + + public function testReverseYieldsAnInverseBelongsToManyPreservingTheJunctionAndSwappingTheKeys() + { + $source = new Book(); + $resolver = (new Query())->setModel($source)->getResolver(); + // Book->author: many-to-many through a plain junction with explicit keys; Author declares no inverse, + // so it is created eagerly during reversal (which is where the key pairs must be exchanged) + $forward = $resolver->getRelations($source)->get('author')->bindTo($source, 'book.author', $resolver); + + $reversed = iterator_to_array($forward->reverse($resolver)); + + $this->assertCount(1, $reversed); + $inverse = $reversed[0]; + + $this->assertInstanceOf(BelongsToMany::class, $inverse); + $this->assertSame('book', $inverse->getName()); + $this->assertSame($source, $inverse->getTarget()); + + // The junction is preserved ... + $this->assertSame($forward->getThroughClass(), $inverse->getThroughClass()); + $this->assertSame($forward->getThroughAlias(), $inverse->getThroughAlias()); + + // ... and the source-side and target-side key pairs are exchanged as a whole + $this->assertSame($forward->getTargetCandidateKey(), $inverse->getCandidateKey()); + $this->assertSame($forward->getTargetForeignKey(), $inverse->getForeignKey()); + $this->assertSame($forward->getCandidateKey(), $inverse->getTargetCandidateKey()); + $this->assertSame($forward->getForeignKey(), $inverse->getTargetForeignKey()); + } } diff --git a/tests/Lib/Model/Author.php b/tests/Lib/Model/Author.php new file mode 100644 index 00000000..34c53af3 --- /dev/null +++ b/tests/Lib/Model/Author.php @@ -0,0 +1,33 @@ +author must create it eagerly, and there is + // no junction model to re-derive the keys from, so the reversed keys come solely from reverse(). + } +} diff --git a/tests/Lib/Model/Book.php b/tests/Lib/Model/Book.php new file mode 100644 index 00000000..f4910dcf --- /dev/null +++ b/tests/Lib/Model/Book.php @@ -0,0 +1,40 @@ +belongsToMany('author', Author::class) + ->through('authorship') + ->setCandidateKey('book_no') // book column + ->setForeignKey('authored_book') // junction column referencing the book + ->setTargetForeignKey('authoring') // junction column referencing the author + ->setTargetCandidateKey('author_ref'); // author column + } +} diff --git a/tests/RelationTest.php b/tests/RelationTest.php index c9ceadb3..17dfc137 100644 --- a/tests/RelationTest.php +++ b/tests/RelationTest.php @@ -2,8 +2,17 @@ namespace ipl\Tests\Orm; +use ipl\Orm\Query; use ipl\Orm\Relation; +use ipl\Orm\Relation\BelongsTo; +use ipl\Orm\Relation\HasMany; +use ipl\Orm\Relation\HasOne; +use ipl\Orm\Resolver; use ipl\Stdlib\Filter; +use ipl\Tests\Orm\Lib\Model\Department; +use ipl\Tests\Orm\Lib\Model\RestrictedUser; +use LogicException; +use RuntimeException; class RelationTest extends \PHPUnit\Framework\TestCase { @@ -206,4 +215,98 @@ public function testResolveYieldsTheRelationItselfAsKey() $this->assertSame([$relation], $keys); } + + public function testGetReverseNameReturnsNullByDefault() + { + $this->assertNull((new Relation())->getReverseName()); + } + + public function testSetReverseNameSetsTheReverseName() + { + $this->assertSame('foo', (new Relation())->setReverseName('foo')->getReverseName()); + } + + public function testGetReverseClassFallsBackToTheRelationsOwnClass() + { + $this->assertSame(Relation::class, (new Relation())->getReverseClass()); + // Subclasses provide sensible defaults + $this->assertSame(BelongsTo::class, (new HasMany())->getReverseClass()); + $this->assertSame(HasMany::class, (new BelongsTo())->getReverseClass()); + } + + public function testSetReverseClassOverridesTheDefault() + { + $this->assertSame( + HasOne::class, + (new BelongsTo())->setReverseClass(HasOne::class)->getReverseClass() + ); + } + + public function testReverseThrowsIfTheRelationIsUnbound() + { + $this->expectException(LogicException::class); + $this->expectExceptionMessage('Cannot reverse an unbound relation'); + + iterator_to_array((new HasMany())->reverse($this->createStub(Resolver::class))); + } + + public function testReverseReusesADeclaredInverseRelation() + { + $source = new Department(); + $resolver = (new Query())->setModel($source)->getResolver(); + // Binding qualifies the filter and registers the target alias, as resolveRelations() would + $forward = $resolver->getRelations($source) + ->get('employee') + ->bindTo($source, 'department.employee', $resolver); + + $resolver->getRelations($forward->getTarget()) + ->get('department') + ->setCandidateKey('office_id'); // Silly, but must be retained + + $reversed = iterator_to_array($forward->reverse($resolver)); + + $this->assertCount(1, $reversed); + $inverse = $reversed[0]; + + // Employee declares a matching belongsTo 'department' (named after the source's table alias) which + // is reused as the inverse and re-targeted at the very source instance + $this->assertSame($resolver->getRelations($forward->getTarget())->get('department'), $inverse); + $this->assertSame('office_id', $inverse->getCandidateKey()); + $this->assertSame('department', $inverse->getName()); + $this->assertSame($source, $inverse->getTarget()); + } + + public function testReverseCreatesAnInverseRelationWhenNoneIsDeclared() + { + $source = new RestrictedUser(); + $resolver = (new Query())->setModel($source)->getResolver(); + // RestrictedGroup declares no relations, so the inverse has to be created eagerly + $forward = $resolver->getRelations($source) + ->get('restricted_group') + ->bindTo($source, 'restricted_user.restricted_group', $resolver); + + $reversed = iterator_to_array($forward->reverse($resolver)); + + $this->assertCount(1, $reversed); + $inverse = $reversed[0]; + + $this->assertInstanceOf(BelongsTo::class, $inverse); + $this->assertSame('restricted_user', $inverse->getName()); + $this->assertSame($source, $inverse->getTarget()); + $this->assertInstanceOf(RestrictedUser::class, $inverse->getTarget()); + } + + public function testReverseThrowsIfADeclaredInverseTargetsAnIncompatibleModel() + { + $source = new Department(); + $forward = (new Query())->setModel($source)->getResolver()->getRelations($source)->get('employee') + ->setSource($source) + // Employee.office targets Office, but the source of this relation is a Department + ->setReverseName('office'); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('is not compatible with the target model of the inverse relation'); + + iterator_to_array($forward->reverse((new Query())->getResolver())); + } } From 696c94707615df8755a4359346a3222419dcb4bd Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 20 Aug 2026 14:35:13 +0200 Subject: [PATCH 04/12] =?UTF-8?q?Query:=20Use=20`Relation::reverse()`=20in?= =?UTF-8?q?stead=20of=20`array=5Freverse`=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since `::reverse()` uses the source's table alias by default as reverse name, a deprecation notice is triggered if the original forward path uses a different name, indicating that it is necessary to use this name as explicit reverse name. fixes #170 --- src/Query.php | 43 ++++++++----- tests/Lib/Model/Department.php | 2 +- tests/VisibilityFilterTest.php | 113 ++++++++++++++++++++++++++++++++- 3 files changed, 138 insertions(+), 20 deletions(-) diff --git a/src/Query.php b/src/Query.php index 745e1795..d83b1019 100644 --- a/src/Query.php +++ b/src/Query.php @@ -651,27 +651,38 @@ public function createSubQuery(Model $target, string $targetPath, ?Model $from = ->setDb($this->getDb()) ->setModel($target); - $sourceParts = array_reverse(explode('.', $targetPath)); - $sourceParts[0] = $target->getTableAlias(); - $subQueryResolver = $subQuery->getResolver(); - $sourcePath = join('.', $sourceParts); - $originalRelations = iterator_to_array($this->getResolver()->resolveRelations($targetPath, $from), false); - foreach ($subQuery->getResolver()->resolveRelations($sourcePath) as $relation) { - $original = array_pop($originalRelations); - - if ($relation instanceof BelongsToMany) { - $relation->setFilter($original->getThroughFilter()); - $relation->setThroughFilter($original->getFilter()); - } else { - $relation->setFilter($original->getFilter()); + $sourceParts = []; + foreach ($this->getResolver()->resolveRelations($targetPath, $from) as $relationPath => $relation) { + $predecessor = array_slice(explode('.', $relationPath), -2, 1)[0]; + foreach ($relation->reverse($subQueryResolver) as $oppositeRelation) { + if ( + $relation->getReverseName() === null + && $predecessor !== $oppositeRelation->getName() + && $oppositeRelation->getName() === $oppositeRelation->getTarget()->getTableAlias() + ) { + trigger_error(sprintf( + 'Relation "%s" still uses the default table alias during reversal.' + . ' Use `%s::setReverseName("%s")` to get rid of this deprecation notice.', + $relationPath, + $relation::class, + $predecessor + ), E_USER_DEPRECATED); + $oppositeRelation->setName($predecessor); + array_unshift($sourceParts, $predecessor); + } else { + array_unshift($sourceParts, $oppositeRelation->getName()); + } } - - $subQueryTarget = $relation->getTarget(); } - $subQuery->utilize($sourcePath); // TODO: Don't join if there's a matching foreign key + array_unshift($sourceParts, $target->getTableAlias()); + $sourcePath = join('.', $sourceParts); + $subQueryTarget = $subQueryResolver->resolveRelation($sourcePath)->getTarget(); + + // Up until here only the required relations are eagerly registered but not used yet + $subQuery->utilize($sourcePath); if (! $link) { $subQuery->columns(array_map(function ($keyName) use ($sourcePath) { diff --git a/tests/Lib/Model/Department.php b/tests/Lib/Model/Department.php index ce72ef13..1c4cdfd7 100644 --- a/tests/Lib/Model/Department.php +++ b/tests/Lib/Model/Department.php @@ -33,7 +33,7 @@ public function createRelations(Relations $relations) // Relation filter referencing the target (default) and the source table alias $relations->hasMany('lead', Employee::class) ->setFilter(Filter::all( - Filter::equal('employee.role', 'lead'), + Filter::equal('role', 'lead'), Filter::equal('department.name', 'Engineering') )); } diff --git a/tests/VisibilityFilterTest.php b/tests/VisibilityFilterTest.php index 26a7fcbc..10e7f958 100644 --- a/tests/VisibilityFilterTest.php +++ b/tests/VisibilityFilterTest.php @@ -5,6 +5,7 @@ use ipl\Orm\Query; use ipl\Sql\Test\SqlAssertions; use ipl\Stdlib\Filter; +use ipl\Tests\Orm\Lib\Model\Book; use ipl\Tests\Orm\Lib\Model\Department; use ipl\Tests\Orm\Lib\Model\Node; use ipl\Tests\Orm\Lib\Model\RestrictedGroup; @@ -108,6 +109,27 @@ public function testSelfReferencingRelationFilterIsAppliedToTheTarget() ); } + public function testSelfReferencingRelationFilterCanBeFilteredByItsName() + { + $query = Node::on(new TestConnection()) + ->columns('name') + ->filter(Filter::equal('child.name', 'John Doe')); + + $this->assertSql( + <<<'SQL' + SELECT node.name + FROM node + WHERE (node.deleted = ?) AND (node.id IN ((SELECT sub_node_node.id AS sub_node_node_id + FROM node sub_node + INNER JOIN node sub_node_node ON (sub_node_node.id = sub_node.parent_id) + AND ((sub_node.name = ?) AND (sub_node_node.deleted = ?)) + WHERE (sub_node.deleted = ?) AND (sub_node.name = ?)))) + SQL, + $query->assembleSelect(), + ['n', 'foo', 'n', 'n', 'John Doe'] + ); + } + public function testRelationFilterIsAppliedToJoinCondition() { $query = (new Query()) @@ -207,14 +229,54 @@ public function testBelongsToManyThroughAndRelationFiltersAreAppliedToReversedJo FROM car sub_car INNER JOIN car_user sub_car_car_user ON (sub_car_car_user.car_id = sub_car.id) - AND (sub_car.manufacturer = ?) + AND (sub_car_car_user.user_id = ?) INNER JOIN restricted_user sub_car_restricted_user ON (sub_car_restricted_user.id = sub_car_car_user.restricted_user_id) - AND (sub_car_car_user.user_id = ?) + AND (sub_car.manufacturer = ?) WHERE sub_car.model_name = ?)) SQL, $query->assembleSelect(), - ['Icinga', 5, 'volkswagen'] + [5, 'Icinga', 'volkswagen'] + ); + } + + public function testBelongsToManyWithMandatoryKeysJoinsCorrectly() + { + // Sanity anchor for the forward direction of the reversal regression below + $query = (new Query()) + ->setModel(new Book()) + ->columns('title') + ->utilize('author'); + + $this->assertSql( + 'SELECT book.title FROM book' + . ' INNER JOIN authorship book_authorship ON book_authorship.authored_book = book.book_no' + . ' INNER JOIN author book_author ON book_author.author_ref = book_authorship.authoring', + $query->assembleSelect() + ); + } + + public function testBelongsToManyWithMandatoryKeysReversesThemCorrectlyInASubQuery() + { + // Book->author uses a plain junction and non-conventional keys that must be declared explicitly. + // When reversed for the sub-query there is no junction model or default to re-derive them from, so + // the source-side and target-side key pairs must be exchanged as a whole. Regression for the bug + // where BelongsToMany::reverse() only flipped candidate<->foreign within each side. + $query = (new Query()) + ->setDb(new TestConnection()) + ->setModel(new Book()) + ->columns('title') + ->filter(Filter::equal('author.name', 'x')); + + $this->assertSql( + 'SELECT book.title FROM book WHERE book.book_no IN ((SELECT' + . ' sub_author_book.book_no AS sub_author_book_book_no FROM author sub_author' + . ' INNER JOIN authorship sub_author_authorship' + . ' ON sub_author_authorship.authoring = sub_author.author_ref' + . ' INNER JOIN book sub_author_book ON sub_author_book.book_no = sub_author_authorship.authored_book' + . ' WHERE sub_author.name = ?))', + $query->assembleSelect(), + ['x'] ); } @@ -336,6 +398,51 @@ public function testDeriveAppliesARelationFilterThatReferencesTheSourceTable() ); } + public function testSubQueryReversalEmitsADeprecationWhenARelationUsesTheDefaultReverseName() + { + // Filtering through "lead" (whose name differs from its target's table alias "employee") into the + // deeper to-many "ticket" reverses two hops. Reversing the ticket hop falls back to the source's + // table alias ("employee"), which differs from the path segment ("lead"), so a deprecation nudges + // towards setReverseName(). The produced SQL still uses the path segment for backwards compatibility. + $query = (new Query()) + ->setDb(new TestConnection()) + ->setModel(new Department()) + ->columns('name') + ->filter(Filter::equal('lead.ticket.subject', 'x')); + + $deprecations = []; + set_error_handler(function ($_, $message) use (&$deprecations) { + $deprecations[] = $message; + + return true; + }, E_USER_DEPRECATED); + + try { + $select = $query->assembleSelect(); + } finally { + restore_error_handler(); + } + + $this->assertNotEmpty($deprecations, 'Reversal did not emit a deprecation'); + $this->assertStringContainsString( + 'Relation "department.lead.ticket" still uses the default table alias during reversal', + $deprecations[0] + ); + + $this->assertSql( + 'SELECT department.name FROM department WHERE department.id IN ((SELECT' + . ' sub_ticket_lead_department.id AS sub_ticket_lead_department_id FROM ticket sub_ticket' + . ' INNER JOIN employee sub_ticket_lead ON (sub_ticket_lead.id = sub_ticket.employee_id)' + . ' AND ((sub_ticket.open = ?) AND (sub_ticket_lead.deleted = ?))' + . ' INNER JOIN department sub_ticket_lead_department' + . ' ON (sub_ticket_lead_department.id = sub_ticket_lead.department_id)' + . ' AND ((sub_ticket_lead.role = ?) AND (sub_ticket_lead_department.name = ?))' + . ' WHERE sub_ticket.subject = ?))', + $select, + ['y', 'n', 'lead', 'Engineering', 'x'] + ); + } + public function testModelVisibilityFilterColumnsAreNotValidated() { // Unlike relation filters, a model's visibility filter is not validated against selectable columns; From a7d8f3cb8961800d8541152ed239813a78826bf2 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 1 Sep 2026 15:04:47 +0200 Subject: [PATCH 05/12] reverse-intro --- src/Relation.php | 2 +- src/Relation/BelongsToMany.php | 6 +++- tests/BelongsToManyTest.php | 61 ++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 2 deletions(-) diff --git a/src/Relation.php b/src/Relation.php index 578463ca..a358966d 100644 --- a/src/Relation.php +++ b/src/Relation.php @@ -513,7 +513,7 @@ public function reverse(Resolver $resolver): Generator $targetRelations = $resolver->getRelations($this->getTarget()); if ($targetRelations->has($reverseName)) { // Explicit reverse relations must be properly set up with corresponding key pairs - $relation = $targetRelations->get($reverseName); + $relation = clone $targetRelations->get($reverseName); if (! $this->getSource() instanceof ($relation->getTargetClass())) { throw new RuntimeException(sprintf( diff --git a/src/Relation/BelongsToMany.php b/src/Relation/BelongsToMany.php index 9aa161a6..186858fe 100644 --- a/src/Relation/BelongsToMany.php +++ b/src/Relation/BelongsToMany.php @@ -339,7 +339,7 @@ public function reverse(Resolver $resolver): Generator 'The junction model of the relation "%s" (%s) is not compatible' . ' with the junction model of the inverse relation (%s != %s)', $this->getName(), - get_class($relation->getSource()), + get_class($this->getSource()), $relation->getThroughClass(), $this->getThroughClass() )); @@ -349,6 +349,10 @@ public function reverse(Resolver $resolver): Generator $relation->setThrough($this->getThrough()); $relation->setThroughAlias($this->getThroughAlias()); + // The source table is allowed to reference in a join filter so this must ensure that this works on + // the way back as well. Since the source's instance is kept by parent::reverse() this should be safe. + $relation->addThroughFilterSubjects(...[$relation->getTarget()->getTableAlias() => $relation->getTarget()]); + if (! $this->getThroughFilter()->isEmpty()) { $relation->setThroughFilter(clone $this->getThroughFilter()); } diff --git a/tests/BelongsToManyTest.php b/tests/BelongsToManyTest.php index 0e8a4934..6137ea34 100644 --- a/tests/BelongsToManyTest.php +++ b/tests/BelongsToManyTest.php @@ -9,6 +9,7 @@ use ipl\Sql\Test\SqlAssertions; use ipl\Stdlib\Filter; use ipl\Tests\Orm\Lib\Model\Book; +use RuntimeException; class BelongsToManyTest extends \PHPUnit\Framework\TestCase { @@ -133,6 +134,44 @@ public function testSetThroughFilterWrapsABareConditionInAnAllChain() $this->assertSame([$condition], iterator_to_array($filter)); } + public function testThroughFilterSupportsSourceAndJunctionReferencesAtAllTimes(): void + { + $resolver = new Resolver($this->createStub(Query::class)); + $target = new User(); + $source = new Car(); + + $relation = (new BelongsToMany()) + ->setName('user') + ->setTarget($target) + ->through(CarUser::class) + ->setThroughAlias('my_through') + ->bindTo($source, 'car.user', $resolver); + + $this->assertSame( + [ + 'car' => $source, + 'car_user' => $relation->getThrough(), + 'my_through' => $relation->getThrough() + ], + $relation->getThroughFilterSubjects() + ); + + $reversed = iterator_to_array($relation->reverse($resolver))[0]; + + $newSource = new User(); + $reversed->bindTo($newSource, 'user.car', $resolver); + + $this->assertSame( + [ + 'car' => $source, + 'user' => $newSource, + 'car_user' => $relation->getThrough(), + 'my_through' => $relation->getThrough() + ], + $reversed->getThroughFilterSubjects() + ); + } + public function testResolveYieldsJunctionAndTargetRelationsWithTheirFiltersAndJoinType() { $query = (new Query())->setModel(new Car()); @@ -219,4 +258,26 @@ public function testReverseYieldsAnInverseBelongsToManyPreservingTheJunctionAndS $this->assertSame($forward->getCandidateKey(), $inverse->getTargetCandidateKey()); $this->assertSame($forward->getForeignKey(), $inverse->getTargetForeignKey()); } + + public function testReverseThrowsInCaseTheThroughTableIsDifferent(): void + { + $resolver = new Resolver($this->createStub(Query::class)); + + $relation = (new BelongsToMany()) + ->setName('user') + ->setTargetClass(User::class) + ->through('car_user') + ->bindTo(new Car(), 'car.user', $resolver); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage(sprintf( + 'The junction model of the relation "user" (%s) is not compatible' + . ' with the junction model of the inverse relation (%s != %s)', + Car::class, + CarUser::class, + 'car_user' + )); + + iterator_to_array($relation->reverse($resolver)); + } } From fa7f76f33b03a202c645aa415c0a9ca5523dff02 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 1 Sep 2026 15:08:29 +0200 Subject: [PATCH 06/12] query-reverse-util --- tests/Lib/Model/Loose.php | 32 +++++++++++++++++++++ tests/Lib/Model/Relationship.php | 32 +++++++++++++++++++++ tests/RelationTest.php | 48 ++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+) create mode 100644 tests/Lib/Model/Loose.php create mode 100644 tests/Lib/Model/Relationship.php diff --git a/tests/Lib/Model/Loose.php b/tests/Lib/Model/Loose.php new file mode 100644 index 00000000..c00caf80 --- /dev/null +++ b/tests/Lib/Model/Loose.php @@ -0,0 +1,32 @@ +hasMany('relationship', Relationship::class) + ->setForeignKey('coupler') + ->setCandidateKey('coupler') + ->setJoinType('LEFT'); + } +} diff --git a/tests/Lib/Model/Relationship.php b/tests/Lib/Model/Relationship.php new file mode 100644 index 00000000..2b1bde32 --- /dev/null +++ b/tests/Lib/Model/Relationship.php @@ -0,0 +1,32 @@ +hasMany('loose', Loose::class) + ->setForeignKey('coupler') + ->setCandidateKey('coupler') + ->setJoinType('LEFT'); + } +} diff --git a/tests/RelationTest.php b/tests/RelationTest.php index 17dfc137..9e3db5f9 100644 --- a/tests/RelationTest.php +++ b/tests/RelationTest.php @@ -8,14 +8,24 @@ use ipl\Orm\Relation\HasMany; use ipl\Orm\Relation\HasOne; use ipl\Orm\Resolver; +use ipl\Sql\Connection; +use ipl\Sql\Test\SqlAssertions; use ipl\Stdlib\Filter; use ipl\Tests\Orm\Lib\Model\Department; +use ipl\Tests\Orm\Lib\Model\Loose; use ipl\Tests\Orm\Lib\Model\RestrictedUser; use LogicException; use RuntimeException; class RelationTest extends \PHPUnit\Framework\TestCase { + use SqlAssertions; + + public function setUp(): void + { + $this->setUpSqlAssertions(); + } + public function testGetNameReturnsNullIfUnset() { $this->assertNull((new Relation())->getName()); @@ -276,6 +286,44 @@ public function testReverseReusesADeclaredInverseRelation() $this->assertSame($source, $inverse->getTarget()); } + public function testADeclaredInverseRelationCanBeReusedDuringReverse() + { + $connection = $this->createMock(Connection::class); + $connection->method('select')->willReturnCallback(function() { + $stmt = $this->createMock(\PDOStatement::class); + $stmt->expects($this->once())->method('setFetchMode')->with(\PDO::FETCH_ASSOC); + $stmt->method('getIterator')->willReturn(new \ArrayIterator([ + ['id' => 1, 'coupler' => 'test'] + ])); + + return $stmt; + }); + + $loose = Loose::on($connection) + ->filter(Filter::equal('id', 1)) + ->columns('id') + ->first(); + + $others = $loose->relationship->filter(Filter::unequal('loose.id', 1)); + + $this->assertSql( + <<<'SQL' + SELECT sub_relationship.id, sub_relationship.coupler + FROM relationship sub_relationship + LEFT JOIN loose sub_relationship_loose ON sub_relationship_loose.coupler = sub_relationship.coupler + WHERE (sub_relationship_loose.id = ?) + AND ((sub_relationship.id NOT IN ((SELECT sub_loose_relationship.id AS sub_loose_relationship_id + FROM loose sub_loose + LEFT JOIN relationship sub_loose_relationship ON sub_loose_relationship.coupler = sub_loose.coupler + WHERE (sub_loose.id = ?) AND (sub_loose_relationship.id IS NOT NULL) + GROUP BY sub_loose_relationship.id + HAVING COUNT(DISTINCT sub_loose.id) >= ?)) OR sub_relationship.id IS NULL)) + SQL, + $others->assembleSelect(), + [1, 1, 1] + ); + } + public function testReverseCreatesAnInverseRelationWhenNoneIsDeclared() { $source = new RestrictedUser(); From b24478123575d0c5be92a63617d4f389206bbca5 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 2 Sep 2026 10:46:38 +0200 Subject: [PATCH 07/12] reverse-intro --- src/Relation.php | 26 ++++++--------- src/Relation/BelongsTo.php | 4 +++ src/Relation/BelongsToMany.php | 61 +++++++++++++++++----------------- src/Relation/HasMany.php | 4 +++ src/Relation/HasOne.php | 4 +++ tests/BelongsToManyTest.php | 9 ++--- tests/RelationTest.php | 14 +++----- 7 files changed, 59 insertions(+), 63 deletions(-) diff --git a/src/Relation.php b/src/Relation.php index a358966d..2ec2b463 100644 --- a/src/Relation.php +++ b/src/Relation.php @@ -12,6 +12,8 @@ /** * Relations represent the connection between models, i.e. the association between rows in one or more tables * on the basis of matching key columns. The relationships are defined using candidate key-foreign key constructs. + * + * @template TReverse of Relation = static */ class Relation { @@ -21,7 +23,7 @@ class Relation /** @var ?string Name of the reversed relation */ protected ?string $reverseName = null; - /** @var ?class-string The class to reverse the relation */ + /** @var ?class-string The class to reverse the relation */ protected ?string $reverseClass = null; /** @var Model Source model */ @@ -155,7 +157,7 @@ public function setReverseName(string $name): static /** * Get the class to reverse the relation * - * @return class-string + * @return class-string */ public function getReverseClass(): string { @@ -165,7 +167,7 @@ public function getReverseClass(): string /** * Set the class to reverse the relation * - * @param class-string $reverseClass + * @param class-string $reverseClass * * @return $this */ @@ -490,17 +492,17 @@ public function resolve(): Generator /** * Reverse the relation * - * Uses the passed resolver to eagerly register missing relations on the reversed path. + * Uses the passed resolver to eagerly create a relation on the reversed path. Either way, + * the result is still unknown to the given resolver and must be registered with it. * * @param Resolver $resolver * - * @return Generator - * @phpstan-return Generator + * @return TReverse The reversed relation * * @throws LogicException In case the relation is not bound yet (has no source) or has already been reversed * @throws RuntimeException In case the model of the forward relation is incompatible with the reversed relation's */ - public function reverse(Resolver $resolver): Generator + public function reverse(Resolver $resolver): Relation { if ($this->getSource() === null) { throw new LogicException('Cannot reverse an unbound relation.'); @@ -550,14 +552,6 @@ public function reverse(Resolver $resolver): Generator $relation->setFilter(clone $this->getFilter()); } - yield $relation; - - if (! $targetRelations->has($relation->getName())) { - /** - * This is done after `yield` so that the backwards compatibility branch - * of {@see Query::createSubQuery()} is able to change the name. - */ - $targetRelations->add($relation); - } + return $relation; } } diff --git a/src/Relation/BelongsTo.php b/src/Relation/BelongsTo.php index 61419fce..7feb7976 100644 --- a/src/Relation/BelongsTo.php +++ b/src/Relation/BelongsTo.php @@ -6,6 +6,10 @@ /** * Inverse of a one-to-one or one-to-many relationship + * + * @template TReverse of Relation = HasMany + * + * @extends Relation */ class BelongsTo extends Relation { diff --git a/src/Relation/BelongsToMany.php b/src/Relation/BelongsToMany.php index 186858fe..18171683 100644 --- a/src/Relation/BelongsToMany.php +++ b/src/Relation/BelongsToMany.php @@ -331,43 +331,42 @@ public function resolve(): Generator yield from $toTarget->resolve(); } - public function reverse(Resolver $resolver): Generator + public function reverse(Resolver $resolver): Relation { - foreach (parent::reverse($resolver) as $relation) { - if ($relation->getThroughClass() !== null && $relation->getThroughClass() !== $this->getThroughClass()) { - throw new RuntimeException(sprintf( - 'The junction model of the relation "%s" (%s) is not compatible' - . ' with the junction model of the inverse relation (%s != %s)', - $this->getName(), - get_class($this->getSource()), - $relation->getThroughClass(), - $this->getThroughClass() - )); - } - - $relation->through($this->getThroughClass()); - $relation->setThrough($this->getThrough()); - $relation->setThroughAlias($this->getThroughAlias()); + $relation = parent::reverse($resolver); + if ($relation->getThroughClass() !== null && $relation->getThroughClass() !== $this->getThroughClass()) { + throw new RuntimeException(sprintf( + 'The junction model of the relation "%s" (%s) is not compatible' + . ' with the junction model of the inverse relation (%s != %s)', + $this->getName(), + get_class($this->getSource()), + $relation->getThroughClass(), + $this->getThroughClass() + )); + } - // The source table is allowed to reference in a join filter so this must ensure that this works on - // the way back as well. Since the source's instance is kept by parent::reverse() this should be safe. - $relation->addThroughFilterSubjects(...[$relation->getTarget()->getTableAlias() => $relation->getTarget()]); + $relation->through($this->getThroughClass()); + $relation->setThrough($this->getThrough()); + $relation->setThroughAlias($this->getThroughAlias()); - if (! $this->getThroughFilter()->isEmpty()) { - $relation->setThroughFilter(clone $this->getThroughFilter()); - } + // The source table is allowed to reference in a join filter so this must ensure that this works on + // the way back as well. Since the source's instance is kept by parent::reverse() this should be safe. + $relation->addThroughFilterSubjects(...[$relation->getTarget()->getTableAlias() => $relation->getTarget()]); - yield $relation; + if (! $this->getThroughFilter()->isEmpty()) { + $relation->setThroughFilter(clone $this->getThroughFilter()); + } - if (! $resolver->getRelations($this->getTarget())->has($relation->getName())) { - // The relation is eagerly set up and thus needs proper key pairs, - // but reversed as only the forward relation's pairs are known. - $relation->setCandidateKey($this->getTargetCandidateKey()); - $relation->setForeignKey($this->getTargetForeignKey()); - $relation->setTargetCandidateKey($this->getCandidateKey()); - $relation->setTargetForeignKey($this->getForeignKey()); - } + if (! $resolver->getRelations($this->getTarget())->has($relation->getName())) { + // The relation is eagerly set up and thus needs proper key pairs, + // but reversed as only the forward relation's pairs are known. + $relation->setCandidateKey($this->getTargetCandidateKey()); + $relation->setForeignKey($this->getTargetForeignKey()); + $relation->setTargetCandidateKey($this->getCandidateKey()); + $relation->setTargetForeignKey($this->getForeignKey()); } + + return $relation; } protected function extractKey(array $possibleKey): string|array|null diff --git a/src/Relation/HasMany.php b/src/Relation/HasMany.php index 82ca669e..e4f8d1b0 100644 --- a/src/Relation/HasMany.php +++ b/src/Relation/HasMany.php @@ -6,6 +6,10 @@ /** * One-to-many relationship + * + * @template TReverse of Relation = BelongsTo + * + * @extends Relation */ class HasMany extends Relation { diff --git a/src/Relation/HasOne.php b/src/Relation/HasOne.php index fe56aa4b..324670fe 100644 --- a/src/Relation/HasOne.php +++ b/src/Relation/HasOne.php @@ -6,6 +6,10 @@ /** * One-to-one relationship + * + * @template TReverse of Relation = BelongsTo + * + * @extends Relation */ class HasOne extends Relation { diff --git a/tests/BelongsToManyTest.php b/tests/BelongsToManyTest.php index 6137ea34..4be3069d 100644 --- a/tests/BelongsToManyTest.php +++ b/tests/BelongsToManyTest.php @@ -156,7 +156,7 @@ public function testThroughFilterSupportsSourceAndJunctionReferencesAtAllTimes() $relation->getThroughFilterSubjects() ); - $reversed = iterator_to_array($relation->reverse($resolver))[0]; + $reversed = $relation->reverse($resolver); $newSource = new User(); $reversed->bindTo($newSource, 'user.car', $resolver); @@ -239,10 +239,7 @@ public function testReverseYieldsAnInverseBelongsToManyPreservingTheJunctionAndS // so it is created eagerly during reversal (which is where the key pairs must be exchanged) $forward = $resolver->getRelations($source)->get('author')->bindTo($source, 'book.author', $resolver); - $reversed = iterator_to_array($forward->reverse($resolver)); - - $this->assertCount(1, $reversed); - $inverse = $reversed[0]; + $inverse = $forward->reverse($resolver); $this->assertInstanceOf(BelongsToMany::class, $inverse); $this->assertSame('book', $inverse->getName()); @@ -278,6 +275,6 @@ public function testReverseThrowsInCaseTheThroughTableIsDifferent(): void 'car_user' )); - iterator_to_array($relation->reverse($resolver)); + $relation->reverse($resolver); } } diff --git a/tests/RelationTest.php b/tests/RelationTest.php index 9e3db5f9..2200ec44 100644 --- a/tests/RelationTest.php +++ b/tests/RelationTest.php @@ -257,7 +257,7 @@ public function testReverseThrowsIfTheRelationIsUnbound() $this->expectException(LogicException::class); $this->expectExceptionMessage('Cannot reverse an unbound relation'); - iterator_to_array((new HasMany())->reverse($this->createStub(Resolver::class))); + (new HasMany())->reverse($this->createStub(Resolver::class)); } public function testReverseReusesADeclaredInverseRelation() @@ -273,10 +273,7 @@ public function testReverseReusesADeclaredInverseRelation() ->get('department') ->setCandidateKey('office_id'); // Silly, but must be retained - $reversed = iterator_to_array($forward->reverse($resolver)); - - $this->assertCount(1, $reversed); - $inverse = $reversed[0]; + $inverse = $forward->reverse($resolver); // Employee declares a matching belongsTo 'department' (named after the source's table alias) which // is reused as the inverse and re-targeted at the very source instance @@ -333,10 +330,7 @@ public function testReverseCreatesAnInverseRelationWhenNoneIsDeclared() ->get('restricted_group') ->bindTo($source, 'restricted_user.restricted_group', $resolver); - $reversed = iterator_to_array($forward->reverse($resolver)); - - $this->assertCount(1, $reversed); - $inverse = $reversed[0]; + $inverse = $forward->reverse($resolver); $this->assertInstanceOf(BelongsTo::class, $inverse); $this->assertSame('restricted_user', $inverse->getName()); @@ -355,6 +349,6 @@ public function testReverseThrowsIfADeclaredInverseTargetsAnIncompatibleModel() $this->expectException(RuntimeException::class); $this->expectExceptionMessage('is not compatible with the target model of the inverse relation'); - iterator_to_array($forward->reverse((new Query())->getResolver())); + $forward->reverse((new Query())->getResolver()); } } From 3a6f418ec9c2b6f7fef3ae6313ef9a6aef6322c5 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 2 Sep 2026 10:48:07 +0200 Subject: [PATCH 08/12] query-reverse-util --- src/Query.php | 56 ++++++++++++++++++++++-------------------- src/Resolver.php | 15 +++++++++++ tests/RelationTest.php | 1 - 3 files changed, 45 insertions(+), 27 deletions(-) diff --git a/src/Query.php b/src/Query.php index d83b1019..8d26b707 100644 --- a/src/Query.php +++ b/src/Query.php @@ -653,33 +653,37 @@ public function createSubQuery(Model $target, string $targetPath, ?Model $from = $subQueryResolver = $subQuery->getResolver(); - $sourceParts = []; - foreach ($this->getResolver()->resolveRelations($targetPath, $from) as $relationPath => $relation) { - $predecessor = array_slice(explode('.', $relationPath), -2, 1)[0]; - foreach ($relation->reverse($subQueryResolver) as $oppositeRelation) { - if ( - $relation->getReverseName() === null - && $predecessor !== $oppositeRelation->getName() - && $oppositeRelation->getName() === $oppositeRelation->getTarget()->getTableAlias() - ) { - trigger_error(sprintf( - 'Relation "%s" still uses the default table alias during reversal.' - . ' Use `%s::setReverseName("%s")` to get rid of this deprecation notice.', - $relationPath, - $relation::class, - $predecessor - ), E_USER_DEPRECATED); - $oppositeRelation->setName($predecessor); - array_unshift($sourceParts, $predecessor); - } else { - array_unshift($sourceParts, $oppositeRelation->getName()); - } + $forwardHops = array_slice(explode('.', $targetPath), 0, -1); + $forwardRelations = iterator_to_array($this->getResolver()->resolveRelations($targetPath, $from)); + + $sourceHops = [$target->getTableAlias()]; + foreach (array_reverse($forwardRelations) as $forwardPath => $relation) { + $oppositeRelation = $relation->reverse($subQueryResolver); + + $predecessor = array_pop($forwardHops); + if ($relation->getReverseName() === null && $predecessor !== $oppositeRelation->getName()) { + trigger_error(sprintf( + 'Relation "%s" still uses the default table alias during reversal.' + . ' Use `%s::setReverseName("%s")` to get rid of this deprecation notice.', + $forwardPath, + $relation::class, + $predecessor + ), E_USER_DEPRECATED); + $oppositeRelation->setName($predecessor); } + + /** + * This reduces available relations on the reverse path to what is actually needed for the outer + * query join. This is fine right now, since {@see Compat\FilterProcessor::requireAndResolveFilterColumns} + * will utilize separate sub queries for individual relations at the moment. + */ + $subQueryResolver->setRelations($relation->getTarget(), (new Relations())->add($oppositeRelation)); + + $target = $oppositeRelation->getTarget(); + $sourceHops[] = $oppositeRelation->getName(); } - array_unshift($sourceParts, $target->getTableAlias()); - $sourcePath = join('.', $sourceParts); - $subQueryTarget = $subQueryResolver->resolveRelation($sourcePath)->getTarget(); + $sourcePath = join('.', $sourceHops); // Up until here only the required relations are eagerly registered but not used yet $subQuery->utilize($sourcePath); @@ -687,7 +691,7 @@ public function createSubQuery(Model $target, string $targetPath, ?Model $from = if (! $link) { $subQuery->columns(array_map(function ($keyName) use ($sourcePath) { return "$sourcePath.$keyName"; - }, (array) $subQueryTarget->getKeyName())); + }, (array) $target->getKeyName())); return $subQuery; } @@ -698,7 +702,7 @@ public function createSubQuery(Model $target, string $targetPath, ?Model $from = $resolver = $this->getResolver(); $baseAlias = $resolver->getAlias($this->getModel()); - $sourceAlias = $subQueryResolver->getAlias($subQueryTarget); + $sourceAlias = $subQueryResolver->getAlias($target); $subQueryConditions = []; foreach ((array) $this->getModel()->getKeyName() as $column) { diff --git a/src/Resolver.php b/src/Resolver.php index fb2fb862..17c492a2 100644 --- a/src/Resolver.php +++ b/src/Resolver.php @@ -75,6 +75,21 @@ public function __construct(Query $query) $this->visibilityFilters = new SplObjectStorage(); } + /** + * Override a model's default relations with the given ones + * + * @param Model $model + * @param Relations $relations + * + * @return $this + */ + public function setRelations(Model $model, Relations $relations): static + { + $this->relations->offsetSet($model, $relations); + + return $this; + } + /** * Get a model's relations * diff --git a/tests/RelationTest.php b/tests/RelationTest.php index 2200ec44..9db31412 100644 --- a/tests/RelationTest.php +++ b/tests/RelationTest.php @@ -277,7 +277,6 @@ public function testReverseReusesADeclaredInverseRelation() // Employee declares a matching belongsTo 'department' (named after the source's table alias) which // is reused as the inverse and re-targeted at the very source instance - $this->assertSame($resolver->getRelations($forward->getTarget())->get('department'), $inverse); $this->assertSame('office_id', $inverse->getCandidateKey()); $this->assertSame('department', $inverse->getName()); $this->assertSame($source, $inverse->getTarget()); From bcd7fd8c1c8a5b68af26e1fede4ce16b88181d32 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 2 Sep 2026 10:50:18 +0200 Subject: [PATCH 09/12] wip --- src/Query.php | 51 ++++++++++++++++++++++++++++++++-- tests/HydratorTest.php | 4 +-- tests/RelationTest.php | 13 ++++----- tests/VisibilityFilterTest.php | 26 ++++++++--------- 4 files changed, 69 insertions(+), 25 deletions(-) diff --git a/src/Query.php b/src/Query.php index 8d26b707..b6a686c2 100644 --- a/src/Query.php +++ b/src/Query.php @@ -22,6 +22,7 @@ use ipl\Stdlib\Filters; use IteratorAggregate; use ReflectionClass; +use RuntimeException; use SplObjectStorage; use Traversable; @@ -622,15 +623,59 @@ public function createHydrator(): Hydrator * @return static<*> * * @throws InvalidArgumentException If the relation with the given name does not exist + * @throws RuntimeException If the reversed relation does not met the expected target */ public function derive($relation, Model $source): static { - // TODO: Think of a way to merge derive() and createSubQuery() - return $this->createSubQuery( - $this->getResolver()->getRelations($source)->get($relation)->getTarget(), + $relation = $this->getResolver()->resolveRelation( $this->getResolver()->qualifyPath($relation, $source->getTableAlias()), $source ); + $query = $relation->getTargetClass()::on($this->getDb()); + $resolver = $query->getResolver(); + $reversed = $relation->reverse($resolver); + + $newRelations = new Relations(); + + + // TODO: This is still the culprit why the test \ipl\Tests\Orm\RelationTest::testADeclaredInverseRelationCanBeReusedDuringReverse fails, + // but it's required for other tests to succeed. See who's right… + $newRelations->add($reversed); + + + foreach ($resolver->getRelations($query->getModel()) as $sibling) { + if ($sibling->getName() !== $reversed->getName()) { + $newRelations->add($sibling); + } + } + + $resolver->setRelations($query->getModel(), $newRelations); + $reversed->bindTo($query->getModel(), $reversed->getName(), $resolver); + + $relatedKeys = null; + foreach ($reversed->resolve() as [$_, $target, $relatedKeys]) { + if ($target === $relation->getTarget()) { + break; + } + } + + if ($relatedKeys === null) { + throw new RuntimeException(sprintf( + 'Reversed relation "%s" (%s) does not resolve to the expected target: %s)', + $relation->getName(), + get_class($source), + $relation->getTargetClass() + )); + } + + foreach ($relatedKeys as $fk => $_) { + $query->filter(Filter::equal( + sprintf('%s.%s', $reversed->getName(), $fk), + $source->$fk + )); + } + + return $query; } /** diff --git a/tests/HydratorTest.php b/tests/HydratorTest.php index 56368df6..bc3a7ef1 100644 --- a/tests/HydratorTest.php +++ b/tests/HydratorTest.php @@ -30,10 +30,10 @@ public function testWhetherProperlyQualifiedColumnsAreOnlyPassedOnToMatchingTarg $hydrator = $query->createHydrator(); - $subject = new Car(); + $subject = new Car(['id' => 1]); $hydrator->hydrate(['car_user_custom_keys_username' => 'foo'], $subject); - $subject2 = new Car(); + $subject2 = new Car(['id' => 2]); $hydrator->hydrate(['car_user_custom_keys_username' => 'bar'], $subject2); $this->assertFalse( diff --git a/tests/RelationTest.php b/tests/RelationTest.php index 9db31412..a633e1b0 100644 --- a/tests/RelationTest.php +++ b/tests/RelationTest.php @@ -304,19 +304,18 @@ public function testADeclaredInverseRelationCanBeReusedDuringReverse() $this->assertSql( <<<'SQL' - SELECT sub_relationship.id, sub_relationship.coupler - FROM relationship sub_relationship - LEFT JOIN loose sub_relationship_loose ON sub_relationship_loose.coupler = sub_relationship.coupler - WHERE (sub_relationship_loose.id = ?) - AND ((sub_relationship.id NOT IN ((SELECT sub_loose_relationship.id AS sub_loose_relationship_id + SELECT relationship.id, relationship.coupler + FROM relationship + WHERE (relationship.coupler = ?) + AND ((relationship.id NOT IN ((SELECT sub_loose_relationship.id AS sub_loose_relationship_id FROM loose sub_loose LEFT JOIN relationship sub_loose_relationship ON sub_loose_relationship.coupler = sub_loose.coupler WHERE (sub_loose.id = ?) AND (sub_loose_relationship.id IS NOT NULL) GROUP BY sub_loose_relationship.id - HAVING COUNT(DISTINCT sub_loose.id) >= ?)) OR sub_relationship.id IS NULL)) + HAVING COUNT(DISTINCT sub_loose.id) >= ?)) OR relationship.id IS NULL)) SQL, $others->assembleSelect(), - [1, 1, 1] + ['test', 1, 1] ); } diff --git a/tests/VisibilityFilterTest.php b/tests/VisibilityFilterTest.php index 10e7f958..00113db5 100644 --- a/tests/VisibilityFilterTest.php +++ b/tests/VisibilityFilterTest.php @@ -364,12 +364,12 @@ public function testDeriveAppliesTheModelVisibilityFilterAndTheRelationFilter() $derived = $query->derive('employee', new Department(['id' => 1])); $this->assertSql( - 'SELECT sub_employee.id, sub_employee.name, sub_employee.active, sub_employee.deleted,' - . ' sub_employee.role, sub_employee.department_id, sub_employee.office_id' - . ' FROM employee sub_employee' - . ' INNER JOIN department sub_employee_department' - . ' ON (sub_employee_department.id = sub_employee.department_id) AND (sub_employee.active = ?)' - . ' WHERE (sub_employee.deleted = ?) AND (sub_employee_department.id = ?)', + 'SELECT employee.id, employee.name, employee.active, employee.deleted,' + . ' employee.role, employee.department_id, employee.office_id' + . ' FROM employee' + . ' INNER JOIN department employee_department' + . ' ON (employee_department.id = employee.department_id) AND (employee.active = ?)' + . ' WHERE (employee.deleted = ?) AND (employee_department.id = ?)', $derived->assembleSelect(), ['y', 'n', 1] ); @@ -386,13 +386,13 @@ public function testDeriveAppliesARelationFilterThatReferencesTheSourceTable() $derived = $query->derive('lead', new Department(['id' => 1])); $this->assertSql( - 'SELECT sub_employee.id, sub_employee.name, sub_employee.active, sub_employee.deleted,' - . ' sub_employee.role, sub_employee.department_id, sub_employee.office_id' - . ' FROM employee sub_employee' - . ' INNER JOIN department sub_employee_department' - . ' ON (sub_employee_department.id = sub_employee.department_id)' - . ' AND ((sub_employee.role = ?) AND (sub_employee_department.name = ?))' - . ' WHERE (sub_employee.deleted = ?) AND (sub_employee_department.id = ?)', + 'SELECT employee.id, employee.name, employee.active, employee.deleted,' + . ' employee.role, employee.department_id, employee.office_id' + . ' FROM employee' + . ' INNER JOIN department employee_department' + . ' ON (employee_department.id = employee.department_id)' + . ' AND ((employee.role = ?) AND (employee_department.name = ?))' + . ' WHERE (employee.deleted = ?) AND (employee_department.id = ?)', $derived->assembleSelect(), ['lead', 'Engineering', 'n', 1] ); From 70241d2388257a4fa749e0187ef71ecec29b9da3 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 3 Sep 2026 09:33:51 +0200 Subject: [PATCH 10/12] wip --- src/Query.php | 34 ++++++++++++++++++---------------- src/Relation.php | 8 ++++---- tests/RelationTest.php | 3 ++- tests/VisibilityFilterTest.php | 14 +++++++------- 4 files changed, 31 insertions(+), 28 deletions(-) diff --git a/src/Query.php b/src/Query.php index b6a686c2..efb9fea1 100644 --- a/src/Query.php +++ b/src/Query.php @@ -7,7 +7,10 @@ use InvalidArgumentException; use ipl\Orm\Common\SortUtil; use ipl\Orm\Compat\FilterProcessor; +use ipl\Orm\Relation\BelongsTo; use ipl\Orm\Relation\BelongsToMany; +use ipl\Orm\Relation\BelongsToOne; +use ipl\Orm\Relation\HasOne; use ipl\Sql\Connection; use ipl\Sql\ExpressionInterface; use ipl\Sql\LimitOffset; @@ -617,6 +620,8 @@ public function createHydrator(): Hydrator /** * Derive a new query to load the specified relation from a concrete model * + * The passed source can be referenced in filters using the `self` relation path. + * * @param string $relation * @param TRow $source * @@ -627,29 +632,26 @@ public function createHydrator(): Hydrator */ public function derive($relation, Model $source): static { - $relation = $this->getResolver()->resolveRelation( + $relation = clone $this->getResolver()->resolveRelation( $this->getResolver()->qualifyPath($relation, $source->getTableAlias()), $source ); + $relation + ->setReverseName('self') // TODO: Add a test that uses this in a filter + ->setReverseClass(match (get_class($relation)) { + BelongsToMany::class, BelongsToOne::class => BelongsToOne::class, + BelongsTo::class => HasOne::class, + default => BelongsTo::class + }); + $query = $relation->getTargetClass()::on($this->getDb()); $resolver = $query->getResolver(); - $reversed = $relation->reverse($resolver); - - $newRelations = new Relations(); - - // TODO: This is still the culprit why the test \ipl\Tests\Orm\RelationTest::testADeclaredInverseRelationCanBeReusedDuringReverse fails, - // but it's required for other tests to succeed. See who's right… - $newRelations->add($reversed); - - - foreach ($resolver->getRelations($query->getModel()) as $sibling) { - if ($sibling->getName() !== $reversed->getName()) { - $newRelations->add($sibling); - } - } + $reversed = $relation->reverse($resolver) + ->setJoinType('INNER'); - $resolver->setRelations($query->getModel(), $newRelations); + // This will fail if the name is occupied, but that's fine… + $resolver->getRelations($query->getModel())->add($reversed); $reversed->bindTo($query->getModel(), $reversed->getName(), $resolver); $relatedKeys = null; diff --git a/src/Relation.php b/src/Relation.php index 2ec2b463..3c83d0c9 100644 --- a/src/Relation.php +++ b/src/Relation.php @@ -512,10 +512,10 @@ public function reverse(Resolver $resolver): Relation $reverseName = $this->getReverseName() ?? $this->getSource()->getTableAlias(); - $targetRelations = $resolver->getRelations($this->getTarget()); - if ($targetRelations->has($reverseName)) { + $relations = $resolver->getRelations($this->getTarget()); + if ($relations->has($reverseName) && is_a($relations->get($reverseName), $this->getReverseClass())) { // Explicit reverse relations must be properly set up with corresponding key pairs - $relation = clone $targetRelations->get($reverseName); + $relation = clone $relations->get($reverseName); if (! $this->getSource() instanceof ($relation->getTargetClass())) { throw new RuntimeException(sprintf( @@ -528,7 +528,7 @@ public function reverse(Resolver $resolver): Relation } } else { // Eagerly create the relation in case it's only necessary during reversal - $relation = $targetRelations->create( + $relation = $relations->create( $this->getReverseClass(), $reverseName, get_class($this->getSource()) diff --git a/tests/RelationTest.php b/tests/RelationTest.php index a633e1b0..9b2f5291 100644 --- a/tests/RelationTest.php +++ b/tests/RelationTest.php @@ -306,7 +306,8 @@ public function testADeclaredInverseRelationCanBeReusedDuringReverse() <<<'SQL' SELECT relationship.id, relationship.coupler FROM relationship - WHERE (relationship.coupler = ?) + INNER JOIN loose relationship_self ON relationship_self.coupler = relationship.coupler + WHERE (relationship_self.coupler = ?) AND ((relationship.id NOT IN ((SELECT sub_loose_relationship.id AS sub_loose_relationship_id FROM loose sub_loose LEFT JOIN relationship sub_loose_relationship ON sub_loose_relationship.coupler = sub_loose.coupler diff --git a/tests/VisibilityFilterTest.php b/tests/VisibilityFilterTest.php index 00113db5..6b040ae0 100644 --- a/tests/VisibilityFilterTest.php +++ b/tests/VisibilityFilterTest.php @@ -367,9 +367,9 @@ public function testDeriveAppliesTheModelVisibilityFilterAndTheRelationFilter() 'SELECT employee.id, employee.name, employee.active, employee.deleted,' . ' employee.role, employee.department_id, employee.office_id' . ' FROM employee' - . ' INNER JOIN department employee_department' - . ' ON (employee_department.id = employee.department_id) AND (employee.active = ?)' - . ' WHERE (employee.deleted = ?) AND (employee_department.id = ?)', + . ' INNER JOIN department employee_self' + . ' ON (employee_self.id = employee.department_id) AND (employee.active = ?)' + . ' WHERE (employee.deleted = ?) AND (employee_self.id = ?)', $derived->assembleSelect(), ['y', 'n', 1] ); @@ -389,10 +389,10 @@ public function testDeriveAppliesARelationFilterThatReferencesTheSourceTable() 'SELECT employee.id, employee.name, employee.active, employee.deleted,' . ' employee.role, employee.department_id, employee.office_id' . ' FROM employee' - . ' INNER JOIN department employee_department' - . ' ON (employee_department.id = employee.department_id)' - . ' AND ((employee.role = ?) AND (employee_department.name = ?))' - . ' WHERE (employee.deleted = ?) AND (employee_department.id = ?)', + . ' INNER JOIN department employee_self' + . ' ON (employee_self.id = employee.department_id)' + . ' AND ((employee.role = ?) AND (employee_self.name = ?))' + . ' WHERE (employee.deleted = ?) AND (employee_self.id = ?)', $derived->assembleSelect(), ['lead', 'Engineering', 'n', 1] ); From 308290917465479b239d815b9bc7d3d30cbd13c1 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 4 Sep 2026 10:24:39 +0200 Subject: [PATCH 11/12] wip --- src/Query.php | 12 +++++++----- tests/QueryTest.php | 12 ++++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/Query.php b/src/Query.php index efb9fea1..30a46462 100644 --- a/src/Query.php +++ b/src/Query.php @@ -703,6 +703,7 @@ public function createSubQuery(Model $target, string $targetPath, ?Model $from = $forwardHops = array_slice(explode('.', $targetPath), 0, -1); $forwardRelations = iterator_to_array($this->getResolver()->resolveRelations($targetPath, $from)); + $previousHop = $target; $sourceHops = [$target->getTableAlias()]; foreach (array_reverse($forwardRelations) as $forwardPath => $relation) { $oppositeRelation = $relation->reverse($subQueryResolver); @@ -724,21 +725,22 @@ public function createSubQuery(Model $target, string $targetPath, ?Model $from = * query join. This is fine right now, since {@see Compat\FilterProcessor::requireAndResolveFilterColumns} * will utilize separate sub queries for individual relations at the moment. */ - $subQueryResolver->setRelations($relation->getTarget(), (new Relations())->add($oppositeRelation)); + $subQueryResolver->setRelations($previousHop, (new Relations())->add($oppositeRelation)); - $target = $oppositeRelation->getTarget(); $sourceHops[] = $oppositeRelation->getName(); + $previousHop = $oppositeRelation->getTarget(); } + unset($previousHop); $sourcePath = join('.', $sourceHops); // Up until here only the required relations are eagerly registered but not used yet - $subQuery->utilize($sourcePath); + $subQueryTarget = $subQuery->utilize($sourcePath)->getResolver()->resolveRelation($sourcePath)->getTarget(); if (! $link) { $subQuery->columns(array_map(function ($keyName) use ($sourcePath) { return "$sourcePath.$keyName"; - }, (array) $target->getKeyName())); + }, (array) $subQueryTarget->getKeyName())); return $subQuery; } @@ -749,7 +751,7 @@ public function createSubQuery(Model $target, string $targetPath, ?Model $from = $resolver = $this->getResolver(); $baseAlias = $resolver->getAlias($this->getModel()); - $sourceAlias = $subQueryResolver->getAlias($target); + $sourceAlias = $subQueryResolver->getAlias($subQueryTarget); $subQueryConditions = []; foreach ((array) $this->getModel()->getKeyName() as $column) { diff --git a/tests/QueryTest.php b/tests/QueryTest.php index 0c009615..abc61e79 100644 --- a/tests/QueryTest.php +++ b/tests/QueryTest.php @@ -607,4 +607,16 @@ public function testWithoutColumnsDoesNotWorkWithExpressions() $query->assembleSelect() ); } + + /** + * This test asserts that passing an unreferenced target model to {@see \ipl\Orm\Query::createSubQuery} + * works without an error, to ensure that path reversal keeps model references intact. + */ + public function testUnreferencedTargetCanBePassedToCreateSubQuery(): void + { + $query = Profile::on(new TestConnection()) + ->createSubQuery(new User(), 'profile.user'); + + $this->assertInstanceOf(Query::class, $query); + } } From f9ce76e7d94363ea16ed09ea3efd44a44b1a1f13 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 4 Sep 2026 16:06:28 +0200 Subject: [PATCH 12/12] query-reverse-util --- tests/RelationTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/RelationTest.php b/tests/RelationTest.php index 9b2f5291..b1a19ef1 100644 --- a/tests/RelationTest.php +++ b/tests/RelationTest.php @@ -285,7 +285,7 @@ public function testReverseReusesADeclaredInverseRelation() public function testADeclaredInverseRelationCanBeReusedDuringReverse() { $connection = $this->createMock(Connection::class); - $connection->method('select')->willReturnCallback(function() { + $connection->method('select')->willReturnCallback(function () { $stmt = $this->createMock(\PDOStatement::class); $stmt->expects($this->once())->method('setFetchMode')->with(\PDO::FETCH_ASSOC); $stmt->method('getIterator')->willReturn(new \ArrayIterator([