5.1: Can't open settings tab of SiteTree objects, if you have a lot of members

[johannesx75]

If you have a large number of members in your database (> 200.000 in our case) and you click on any pages settings tab in the cms, it will fail to load.

The given error changes around. We have seen timeouts and memory exhaustions. Always in different low level functions and without a stack trace.

Steps to reproduce:

1. Add lots of members to your database
2. Open any page in the CMS
3. Click on Settings in the top right corner

After stepping through the issue with xDebug I think the issue is caused by this change: https://github.com/silverstripe/silverstripe-cms/commit/14eb767c9c7b83146c70cbd075288506c94a1e6e

In SiteTree::getSettingsFields the following line $membersMap = Member::get()->map('ID', 'Name'); defines a map of all members. This gets passed on to the constructer of ListboxField for ViewerMembers. From there it eventually ends up in SelectField::getListMap where toArray()is called.

Found on Silverstripe CMS 5.1 with PHP 8.1.24.

Acceptance criteria

• The list of Members in the "Only these users" field is limited to 100.

Note

• We're doing very small fix initially so we can fix this in a patch release. We'll do a more comprehensive fix in a follow up card that will ship in a minor release.
• The 100 limit will functionality render this feature unusable for some sites, but it will allow them to load the page.

PR

• https://github.com/silverstripe/silverstripe-cms/pull/2904
• https://github.com/silverstripe/silverstripe-asset-admin/pull/1421

[GuySartorelli]

For the purposes of discussion, bear in mind that while this specific scenario is with 200k+ records, we don't know where the threshold is. It could be an issue with significantly less records than that, for all we know.

There are a few options for resolving this that I can think of:

1. Add lazy loading to ListboxField - it already has references in its react schema where it's explicitly setting lazyLoad to false, so theoretically the react component already has some mechanism for lazyloading records.
2. Apply some appropriate limit to the list that's provided to ListboxField (will only work if the limit is still applied when filtering by the terms that the user types in, which I'm not convinced is the case)
3. Provide a configuration option to hide the "only these users" option from the access rules
4. Swap the field out for a numeric field if the number of records passes some threshold - this is what the auto-scaffolded dropdown for has_one relations does. It's a bad UX though, so I strongly recommend we don't implement this solution.

We could also implement a combination of these - e.g. provide a configuration property for now, and then work on listbox lazyloading as a future enhancement.

Note that of the suggested solutions above, only 2 and 4 can be released as a patch.

A workaround for now is to simply remove the offending fields in getSettingsFields() in your Page class.

Note that in 5.2 this same problem will also affect SiteConfig.

[andrewandante]

Thanks for this bug report, definitely an oversight on my part.

Just jotting down thoughts to kick off resolution discussions - I don't think Member::get() by itself is the issue, otherwise SecurityAdmin would also be exploding. That means it's likely the ->map() function or the ->toArray() function used inside ListboxField(). Offering an extension hook might be a useful option here too, assuming the above is correct, though filtering on a large dataset is likely not going to help "performance" too much.

I think there are benefits to both option 1 (LazyLoading) and option 3 (configurability) proposed above. Both feel a little fiddly, and I'd say LazyLoading has the most potential for longer-term gains, so I'm leaning that way at the moment. Doing both does feel like a good idea though

[johannesx75]

Regarding the workaround: You would have to overwrite the entire 'getSettingsFields()' if you want to do this in 'Page'. Since the ->toArray() happens during the creation of the fields. Removing them after the fact has no effect.

[GuySartorelli]

Just jotting down thoughts to kick off resolution discussions - I don't think Member::get() by itself is the issue, otherwise SecurityAdmin would also be exploding. That means it's likely the ->map() function or the ->toArray() function used inside ListboxField.

::get() doesn't execute the query - the query isn't executed until one of those methods (not sure if map() executes it but if not, toArray() definitely will). It's the execution of the query, and iterating through all of the resulting records, which causes a problem. If we can limit the result set, in some way (e.g. lazy loading or applying a limit()) that will resolve the problem.

Regarding the workaround: You would have to overwrite the entire 'getSettingsFields()' if you want to do this in 'Page'. Since the ->toArray() happens during the creation of the fields. Removing them after the fact has no effect.

Good point, that's a wrinkle.

[maxime-rainville]

I've added that to our refinement session for next Monday and bumped the impact to high.

If it's not practical to LazyLoad the list, maybe we just give people a way to easily opt-out of it in the short term.

I think the lack of an AJAX driven DropdownField - something like a more generic version of TreeDropdownField - is a short coming we're overdue to address. Maybe we bring in a card around that in the CMS 5.2 milestone.

[johannesx75]

If it's not practical to LazyLoad the list, maybe we just give people a way to easily opt-out of it in the short term.

As a really quick workaround: if you would ad a extension hook after $membersMap = Member::get()->map('ID', 'Name'); to allow adding a ->filter() call to $membersMap than affected installations could limit the user lists by some logik. In our case we would remove all end users. I know, one would have to find the issue first and see the extension hook. But it would offer a solution until something better is found.

[GuySartorelli]

If it's not practical to LazyLoad the list, maybe we just give people a way to easily opt-out of it in the short term.

Both implementing lazyloading and adding a new config or method API are minor release solutions. If we are intending to fix this in a patch, the best short-term solution is probably applying a limit() to the list (number 2 in my suggested solutions above).

As a really quick workaround: if you would ad a extension hook

This would also need to be implemented in a minor release, as per our definition of public API

We can be flexible with that definition when needed, but given there's a possible solution available that can be included in a patch release according to the existing definition, we should probably prefer to do that instead. We could then look at a more customisable solution for 5.2

[GuySartorelli]

@emteknetnz There's no linked PRs - please link all relevant PRs in the description

[emteknetnz]

Sorry, have linked

[GuySartorelli]

@emteknetnz Please also apply this to the asset-admin list which was added at the same time as the one we're handling here. See the original issue and this PR specifically for details. It wasn't mentioned in this issue but it has the same problem.

[emteknetnz]

Done, linked above

[GuySartorelli]

PRs merged, stop-gap solution in place. See https://github.com/silverstripe/silverstripe-admin/issues/1618 for the long-term solution.