Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Don't throw exception for empty eagerloaded relation #11220

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented May 6, 2024

Fixes a bug where eagerloading an empty many_many relation would result in an exception.
It was caused by adding 'AND ' . $childIDField . ' IN (' . $fetchedIDsAsString . ')' to the SQL query for fetching join rows, without checking that we had actually found any child IDs in the first place.

Can be triggered by either having an empty relation list, or by causing the list to be empty by manipulating the eagerloaded query with the new syntax.

Having this in _config.php is enough to reproduce the original bug:

<?php

use SilverStripe\Security\Group;

$items = Group::get()->eagerLoad('Roles');
foreach ($items as $item) {
    // no-op
}

Issue

throw new LogicException("Couldn't find parent for record $fetchedID on $relationType relation $relationName");
throw new LogicException("Couldn't find parent for record {$fetched->ID} on $relationType relation $relationName");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated - I noticed while I was looking at this that the $fetchedID variable doesn't exist.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code look good, though this should target 5.2?

@GuySartorelli GuySartorelli changed the base branch from 5 to 5.2 May 6, 2024 02:19
@GuySartorelli
Copy link
Member Author

Oops! Sure should. Retargeted and reran CI.

@emteknetnz emteknetnz merged commit a198c91 into silverstripe:5.2 May 6, 2024
15 checks passed
@emteknetnz emteknetnz deleted the pulls/5.2/empty-eagerloaded-relation branch May 6, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants