silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

NEW SearchableDropdownField #11057

Closed emteknetnz closed 9 months ago

emteknetnz commented 10 months ago

Issue https://github.com/silverstripe/silverstripe-admin/issues/1618

This PR is derived from silverstripe/tagfield

Note that it differs from tagfield in that it uses "ID" (int) for "Value" rather than "Title" (string) which was fine for simply creating tags, but not appropriate for regular relations. This means this field cannot be used for creating new objects like tagfield can, so it's not a full replacement for tagfield.

Admin PR

For method signatures that don't match parent classes e.g. SingleSelectField it's because they now either have strong return types, or looser params defined in docblocks. This is allowed in PHP via covariance (tighter return types on sublasses) and contravariance (looser param types on subclasses) - https://www.php.net/manual/en/language.oop5.variance.php

GuySartorelli commented 9 months ago

I've only tested this using the "only these users" field so far, but in that scenario the lazy load limit doesn't seem to be applying - I have a lot more than just 10 items displaying.

GuySartorelli commented 9 months ago

The problem mentioned in https://github.com/silverstripe/silverstripe-framework/pull/11057#issuecomment-1850952010 has still not been fixed

GuySartorelli commented 9 months ago

Not sure if this is an admin or a framework PR problem, but with a many_many list, the values aren't saved (or, if they are saved, they at least don't display in the field after saving)

Code:

use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Forms\SearchableMultiDropdownField;
use SilverStripe\Taxonomy\TaxonomyTerm;

class Page extends SiteTree
{
    private static $many_many = [
        'Terms' => TaxonomyTerm::class
    ];

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $fields->addFieldsToTab('Root.Main', [
            SearchableMultiDropdownField::create('Terms', 'Terms', TaxonomyTerm::get(), $this->Terms(), 'Name'),
        ]);
        return $fields;
    }
}

Update: The same thing happens for a has_many relation - doesn't save the values, or if they are saved they at least aren't displaying in the field.

GuySartorelli commented 9 months ago

With a has_one relation, before trying to save any records, the first option is displayed as though it's selected: image (note that "a" here is the name of a taxonomy term in the db)

The has_one relation does correctly save when I select a different option and save the page, so that's good.

emteknetnz commented 9 months ago

With a has_one relation, before trying to save any records, the first option is displayed as though it's selected:

That's the default behaviour of the existing Dropdownfield and I replicated it. It's a really obnoxious default behaviour IMO though can be switched off by just called ->setHasEmptyDefault(true);

emteknetnz commented 9 months ago

the values aren't saved

Your code example worked fine on my computer, the values saved, did you forget to dev build flush?

GuySartorelli commented 9 months ago

the values aren't saved Your code example worked fine on my computer, the values saved, did you forget to dev build flush?

I did not forget - though clearly either I did something wrong, or the latest batches of changes fixed it, because it works now 👍

GuySartorelli commented 9 months ago

With a has_one relation, before trying to save any records, the first option is displayed as though it's selected: That's the default behaviour of the existing Dropdownfield and I replicated it. It's a really obnoxious default behaviour IMO though can be switched off by just called ->setHasEmptyDefault(true);

That's not the expected behaviour in my mind. Just because DropdownField does that doesn't mean this field should. DropdownField isn't explicitly for working with relations. This field is explicitly for working with relations - and as such the expected behaviour changes a little. It should show that there is no relation stored in the field until such a time as the user stores a relation in the field.

My recommendation is to call $this->setHasEmptyDefault(true); in the constructor. This will preserve the functionality to allow the first item as a default value if people really want that, but requires developers to opt into that.

GuySartorelli commented 9 months ago

This still needs to be addressed: https://github.com/silverstripe/silverstripe-framework/pull/11057#issuecomment-1850952010

emteknetnz commented 9 months ago

Have added the $this->setHasEmptyDefault(true) on the single dropdown contructor

Have rebased to get https://github.com/silverstripe/silverstripe-framework/pull/11073/files which solves the lazyLoad limit not being respected

GuySartorelli commented 9 months ago

As discussed in person, please set the default lazy-load limit to 100