make dropdownFieldThreshold configurable on DBForeignKey #7789

andrewandante · 2018-01-23T14:21:34Z

Slightly fixes #7788 by making it a configurable option and adding a description as to why this has changed.

andrewandante · 2018-01-23T14:38:05Z

Updated rightTitle because I noticed it was < not <=

kinglozzer

I’m in favour of taking this in. I’d also suggest adding a note to the PHPDoc for the config var like:

If you are tweaking this value, you should also consider constructing your form field manually rather than allowing it to be scaffolded

kinglozzer · 2018-01-23T14:37:18Z

src/ORM/FieldType/DBForeignKey.php

+     * @config
+     * @var int
+     */
+    private static $dropdownFieldThreshold = 100;


Config vars should always be $lower_case_underscore (except for when they’re not, many_many_extraFields)

snek_case_like_a_good_boi

kinglozzer · 2018-01-23T14:38:09Z

src/ORM/FieldType/DBForeignKey.php

            $field = new DropdownField($this->name, $title, $list->map('ID', $titleField));
            $field->setEmptyString(' ');
        } else {
            $field = new NumericField($this->name, $title);
+            $field->setRightTitle(


I don’t think we should show a message like this to content authors

How about admins? Part of the complaint was visibility over the issue. Happy for it to be wrapped in if ($isAdmin)?

Or maybe just something more generic - remove the bit about the config var maybe. It'll save a fair bit of time for both parties if there is an "error message". A good dev will search the codebase for the error message anyway, right? 😉

robbieaverill · 2018-01-23T20:31:48Z

src/ORM/FieldType/DBForeignKey.php

+            $field->setRightTitle(
+                'This field has been converted to a NumericField as there are at least ' . $threshold
+                . ' related object(s). This can be set with the config variable'
+                . ' SilverStripe\ORM\FieldTypeDBForeignKey::dropdown_field_threshold'


I don't think that this description belongs on the field, since it's for CMS users rather than developers. This message likely to confuse the user more than anything. What do you think?

Oh, I see it was raised by @kinglozzer too. My vote is to remove this and leave the configurable limit in place without this message.

It was intended for phpdoc not for the user, right.

Well, no, not really. The problem raised was that the issue was invisible:

This becomes extremely frustrating from a UX Point of View where the CMS works and looks nice, but if the client adds more than 100 objects then it breaks the experience.

I agree that we shouldn't be exposing the config stuff straight to the user, but it was complicated and not necessarily intuitive to find this particular block of code to figure out what was going on, so I'd argue that some sort of message leaves a much better experience for both the CMS user and the dev.

Even something like "Too many objects for dropdown, fallback used" would help enormously in debugging, and give the user something useful to pass to their dev, who in turn has something to search for when they are looking to fix it.

Otherwise you end up with a field that seems to have mysteriously changed for what is an invisible reason, which is frustrating and confusing.

The other options I see as viable:

move this functionality to DropdownField itself - which is where one would assume that the config var is located if you were to guess

add some specific documentation around the issue and the config var

Ultimately up to you, but I think making this less "magical" is better

dhensby · 2018-01-24T13:17:07Z

Degrading to a numericfield is a UX nightmare. How many content authors know the IDs of the related objects they need off the top of their head? or are able to map the titles to IDs without having to navigate away / inspect URLs, etc?

When clients/users see what used to be a dropdown become a numeric field for no obvious reason the reaction (naturally) is to think something has broken or even someone has changed something (like a dev deploying bad code).

Of course a dropdown that's so large it causes memory exhaustion or locks up the CMS is a worse experience (completely broken vs unfriendly) but we should be looking at a more friendly mid-point solution.

100 rows seems like a very small number (I'd vote to increase the limit to 1000 in this PR) and I support the idea that the CMS shows some kind of message to inform the user why they are experiencing a degraded experience.

tractorcow · 2018-01-24T22:36:28Z

Just make an autocomplete field with ajax endpoint for searching records. We already do this with ListBox / TreeDropdownField; We just need a basic single select, no hierarchy version.

andrewandante · 2018-01-26T11:00:38Z

@tractorcow would you propose this change to DropdownField itself? Or would you prefer to have a AutocompleteDropdownField, or allow ListboxField to setMultiple(false)? Or to set something for just this use-case?

I have to say that the idea of a searchable DropdownField appeals 😄

That being said, this is maybe creeping outside the scope of the PR. I have reduced it down to adding the config field and a user-friendly message - pretty sure the behat failures are unrelated.

kinglozzer · 2018-02-09T15:58:05Z

Short of someone taking the time to build an autocomplete field for core, I think this as good as it’s gonna get right now. I still don’t like showing a dev-related message to CMS authors, but in this context it probably does more good than harm.

@tractorcow Do you agree that this is an improvement? Even if hopefully some kind soul will eventually replace this with an autocomplete field...

tractorcow · 2018-02-11T22:14:15Z

src/ORM/FieldType/DBForeignKey.php

            $field = new DropdownField($this->name, $title, $list->map('ID', $titleField));
            $field->setEmptyString(' ');
        } else {
            $field = new NumericField($this->name, $title);
+            $field->setRightTitle('Too many related objects; fallback field in use');


Should be localised string not fixed english sentence.

tractorcow · 2018-02-12T21:32:19Z

Current behat failure fixed with #7860, not the fault of this PR.

andrewandante force-pushed the pulls/4/DBForeignKey_dropdown_threshold branch from 4276ac4 to 8d84cb3 Compare January 23, 2018 14:37

kinglozzer requested changes Jan 23, 2018

View reviewed changes

andrewandante force-pushed the pulls/4/DBForeignKey_dropdown_threshold branch from 8d84cb3 to 9b9437a Compare January 23, 2018 15:23

robbieaverill reviewed Jan 23, 2018

View reviewed changes

make dropdownFieldThreshold configurable on DBForeignKey

578ee5d

andrewandante force-pushed the pulls/4/DBForeignKey_dropdown_threshold branch from 9b9437a to 578ee5d Compare January 26, 2018 10:16

kinglozzer approved these changes Feb 9, 2018

View reviewed changes

tractorcow suggested changes Feb 11, 2018

View reviewed changes

add translation string for fallback message

79feb19

tractorcow approved these changes Feb 12, 2018

View reviewed changes

tractorcow merged commit 00ff3ba into silverstripe:4 Feb 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make dropdownFieldThreshold configurable on DBForeignKey #7789

make dropdownFieldThreshold configurable on DBForeignKey #7789

andrewandante commented Jan 23, 2018

andrewandante commented Jan 23, 2018

kinglozzer left a comment

kinglozzer Jan 23, 2018

andrewandante Jan 23, 2018

kinglozzer Jan 23, 2018

andrewandante Jan 23, 2018

andrewandante Jan 23, 2018 •

edited

Loading

robbieaverill Jan 23, 2018

robbieaverill Jan 23, 2018

tractorcow Jan 24, 2018

andrewandante Jan 24, 2018

dhensby commented Jan 24, 2018

tractorcow commented Jan 24, 2018

andrewandante commented Jan 26, 2018 •

edited

Loading

kinglozzer commented Feb 9, 2018

tractorcow Feb 11, 2018

tractorcow commented Feb 12, 2018

make dropdownFieldThreshold configurable on DBForeignKey #7789

make dropdownFieldThreshold configurable on DBForeignKey #7789

Conversation

andrewandante commented Jan 23, 2018

andrewandante commented Jan 23, 2018

kinglozzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewandante Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhensby commented Jan 24, 2018

tractorcow commented Jan 24, 2018

andrewandante commented Jan 26, 2018 • edited Loading

kinglozzer commented Feb 9, 2018

Choose a reason for hiding this comment

tractorcow commented Feb 12, 2018

andrewandante Jan 23, 2018 •

edited

Loading

andrewandante commented Jan 26, 2018 •

edited

Loading