Changes required for the dependent dropdown field #11906
+577
−87
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
28 tasks
GuySartorelli
force-pushed
the
pulls/6/dependent-field-stuff
branch
2 times, most recently
from
fe3ff59 to
be9a09a
Compare
GuySartorelli
force-pushed
the
pulls/6/dependent-field-stuff
branch
6 times, most recently
from
a43b4c6 to
4dfc114
Compare
GuySartorelli
force-pushed
the
pulls/6/dependent-field-stuff
branch
from
4dfc114 to
b0525ef
Compare
GuySartorelli
commented
Dec 1, 2025
Member
Author
There was a problem hiding this comment.
The API here was moved into FieldSetTrait
GuySartorelli
commented
Dec 1, 2025
Comment on lines
-130
to
+175
| $this->fieldNameError($field, __FUNCTION__); | ||
| $this->fieldNameError($field, 'saveableFields'); |
Member
Author
There was a problem hiding this comment.
__FUNCTION__ is the anonymous function - we want the name of the method.
GuySartorelli
commented
Dec 1, 2025
Member
Author
There was a problem hiding this comment.
API here is from CompositeField and therefore can't be strictly typed without breaking BC
GuySartorelli
force-pushed
the
pulls/6/dependent-field-stuff
branch
2 times, most recently
from
4f6a6da to
bea37d1
Compare
Currently this is managed through dataFieldByName() and recursiveWalk() - but those both rely on the field with children explicitly being a CompositeField. I don't want to rely on these, because: 1. recalculating the children fields for the new dependency field (see admin PR) may be computationally expensive in some scenarios and we want to avoid calling it too much. 2. allowing access to children fields too widely for the new field will cause all sorts of problems, e.g. calling `replaceField()` or `renameField()` on a FieldList.
CompositeField has API that lets it change between a `div` and a HTML `fieldset`. Moving this into a trait lets other fields behave the same way without having to inherit everything about CompositeField.
This can be useful for checking the current class the dropdown holds, similar to `getSourceObject()` on a tree dropdown
GuySartorelli
force-pushed
the
pulls/6/dependent-field-stuff
branch
from
bea37d1 to
88d27da
Compare
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
There are multiple commits on purpose. DO NOT SQUASH
Adds new
FormField::SCHEMA_DATA_TYPE_STRUCTURAL_CUSTOMconst.Not strictly necessary as we could just refer to it as a hardcoded string in the admin PR - but adding this here seems consistent with the other
SCHEMA_DATA_TYPEconsts (which one day maybe we'll turn into an enum)Adds a new interface and uses it in various places to allow the following behaviours for weirdly-nested fields (in a way that doesn't break that nesting e.g. intentionally doesn't find them for reordering or removing fields):
TreeDropdownField'streeaction)Not strictly necessary but allows for the new dependent composite field to be a div or a fieldset depending on the use case. Nice to have.
Makes testing the admin PR easier and seems like sensible API to add, but I can work around it if there's any controversy about it.
Of these only 2 and 5 are strictly necessary - but the others make the implementation and tests a little cleaner.
I have also raised #11927 to eventually use the new
ChildFieldManagerinterface inMoneyField.Issue