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

Closed

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Jun 11, 2024

Issue #11272

silverstripe/installer ci run with pr - https://github.com/emteknetnz/silverstripe-installer/actions/runs/9473993800 (includes admin behat) (green)

Note the SearchableDropDownField seems like it's broken in an elemental context, I've created a new card for this

Test setup:

MyDataObject.php

<?php

use SilverStripe\ORM\DataObject;

class MyDataObject extends DataObject
{
    private static $table_name = 'MyDataObject';

    private static $db = [
        'Title' => 'Varchar'
    ];

    private static $has_one = [
        'MySubDataObject' => MySubDataObject::class
    ];
}

MySubDataObject

<?php

use SilverStripe\ORM\DataObject;

class MySubDataObject extends DataObject
{
    private static $table_name = 'MySubDataObject';

    private static $db = [
        'Title' => 'Varchar'
    ];

    private static $has_many = [
        'MyDataObject' => MyDataObject::class
    ];
}

ModelAdmin.php

<?php

use SilverStripe\Admin\ModelAdmin;

class MyModelAdmin extends ModelAdmin
{
    private static $url_segment = 'MyModelAdmin';

    private static $menu_title = 'My model admin';

    private static $managed_models = [
        MyDataObject::class,
    ];
}

MySubModelAdmin.php

<?php

use SilverStripe\Admin\ModelAdmin;

class MySubModelAdmin extends ModelAdmin
{
    private static $url_segment = 'MySubModelAdmin';

    private static $menu_title = 'My sub admin';

    private static $managed_models = [
        MySubDataObject::class,
    ];
}

@@ -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

@emteknetnz emteknetnz closed this Jun 12, 2024
@GuySartorelli GuySartorelli deleted the pulls/5.2/lazy-memory branch June 12, 2024 03:38
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.

2 participants