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 perform readonly transformation for lazyloaded searchabledropdown #11297

Conversation

johannesx75
Copy link
Contributor

@johannesx75 johannesx75 commented Jul 2, 2024

Description

performReadonlyTransformation() on a SearchableDropdownField or SearchableMultiDropdownField, will convert the source DataList into an array, which requires pulling data for all records in the list from the database.

This is caused by the performReadonlyTransformation of the underlying SingleSelectField and MultiSelectField calling getSource. To circumvent this a new LazyLoadedLookupField is used. It has the same logic as SearchableDropdownTrait to use a Datalist internally for the Source.

This also contains the fix for #11294, since the wrong initial value of "Title" was messing up the control.

Please note: This solution feels like quite a hack. I really don't like the fact, that getSource is not really returning the source. But I figured as long as SearchableDropdownTrait works this way the performReadonlyTransformation should be similar to it.

Manual testing steps

Given ModelA has_one ModelB and ModelB has_many ModelA:

  • Open ModelB in Model Admin
  • Go to the ModelA Tab
  • Click on Add new Model A
  • A read only Field should be rendered and only one ModelB will be loaded from the Database

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Have tested locally, works well, just a few small changes to make

Not that I've changed the base from 5.2 to 5 (so will be released in CMS 5.3), because this is adding new API so to follow semver it needs to be released in a new minor

You'll need to rebase on top of 5 as there's now a merge-conflict

src/Forms/SearchableDropdownTrait.php Outdated Show resolved Hide resolved
src/Forms/LazyLoadedLookupField.php Outdated Show resolved Hide resolved
src/Forms/LazyLoadedLookupField.php Outdated Show resolved Hide resolved
src/Forms/LazyLoadedLookupField.php Outdated Show resolved Hide resolved
src/Forms/LazyLoadedLookupField.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz changed the base branch from 5.2 to 5 July 3, 2024 02:23
src/Forms/SearchableDropdownTrait.php Outdated Show resolved Hide resolved
@johannesx75 johannesx75 force-pushed the fix-performReadonlyTransformation-for-lazyloaded-searchabledropdown branch from f23bea6 to e1f4516 Compare July 24, 2024 15:09
@johannesx75 johannesx75 force-pushed the fix-performReadonlyTransformation-for-lazyloaded-searchabledropdown branch from e1f4516 to 6988222 Compare July 24, 2024 15:17
@johannesx75
Copy link
Contributor Author

@emteknetnz, sorry for the late response. I was out sick.

Think I've made all the requested changes. One of the checks fails: 'CI / CI / 8.1 mysql57 phplinting (pull_request)'. From what I can tell thats unrelated to my changes.

@emteknetnz emteknetnz merged commit fb7b173 into silverstripe:5 Jul 25, 2024
14 of 15 checks passed
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