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.

Sign up for GitHub

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 Do not load source in SearchableDropdownTrait when lazy-loading #11273

Closed
wants to merge 1 commit into from
Closed

FIX Do not load source in SearchableDropdownTrait when lazy-loading #11273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Forms/SearchableDropdownTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,10 @@ public function setIsSearchable(bool $isSearchable): static
*/
public function getSource(): array
{
// Source will be unused when lazy-loading so just return an empty array
if ($this->getIsLazyLoaded()) {
return [];
Copy link
Member

Choose a reason for hiding this comment

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

This might result in unexpected behaviour down the line, but there's not a lot we can do about that.

The one place that looks like this might cause problems is MultiSelectField::performReadonlyTransformation() - I think that'll need to be updated to use $this->sourceList instead of $this->getSource().

Alternatively a new getSourceList() method could be added and used in most places that currently use getSource() - that's how TagField seems to have gotten around this problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes unfortunately it does seem to break MultiSelectField::performReadonlyTransformation() when lazy loading

I don't think changing that to use $this->sourceList will really fix this issue, because ultimately it needs to be converted to use an array anyway because in readonly mode it's using a LookField, which internally uses arrays.

To fix this it feels like we need to convert a whole bunch of things to use generators to handle very large sets of options. Given there's a workaround to use a numeric field (which was the old fallback) feels like this is more effort than it's worth, at least for now. Feels more like an API break piece of work where we also get rid of a bunch of the convoluted class hierarchy that exists for dropdowns

I'll close this PR

Copy link
Member

Choose a reason for hiding this comment

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

The readonly list only needs to display the items from the list which have actually been selected though, right? i.e. the current value of the field. Surely we can get a readonly formfield that displays the values without loading every single record in the DB into an array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly

However looking at my closed PR in hindsight, it's honestly hackish. getSource() should just return the source and not care if it's lazyloaded or not

}
return $this->getListMap($this->sourceList);
}

Expand Down
24 changes: 24 additions & 0 deletions tests/php/Forms/SearchableDropdownTraitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,4 +217,28 @@ public function testGetSchemaDataDefaults(): void
$this->assertSame('My placeholder', $schema['placeholder']);
$this->assertFalse($schema['searchable']);
}

public function provideGetSource(): array
{
return [
'lazyLoad' => [
'isLazyLoaded' => true,
'expected' => 0,
],
'not lazyLoad' => [
'isLazyLoaded' => false,
'expected' => 3,
],
];
}

/**
* @dataProvider provideGetSource
*/
public function testGetSource(bool $isLazyLoaded, int $expected): void
{
$field = new SearchableDropdownField('MyField', 'MyField', Team::get());
$field->setIsLazyLoaded($isLazyLoaded);
$this->assertCount($expected, $field->getSource());
}
}
Loading