Various changes to support refactoring CMS search filter functionality#11587
Conversation
GuySartorelli
changed the title
GuySartorelli
force-pushed
the
pulls/6.0/refactor-sitetree-filter
branch
3 times, most recently
from
04adae2 to
1183438
Compare
GuySartorelli
force-pushed
the
pulls/6.0/refactor-sitetree-filter
branch
11 times, most recently
from
4421628 to
613653c
Compare
| if ($controller) { | ||
| $this->restoreFormState(); | ||
| } |
There was a problem hiding this comment.
$controller is optional - but needs to be available and set in this form for form state to be recoverable. This is technically a bugfix but in reality we only really run into it when setting up test forms.
| /** | ||
| * Gets a JSON schema representing a form. | ||
| */ | ||
| public function getSchema(HTTPRequest $request): HTTPResponse | ||
| { | ||
| $schemaID = $request->getURL(); | ||
| $parts = $request->getHeader(FormSchema::SCHEMA_HEADER); | ||
| $data = FormSchema::singleton()->getMultipartSchema($parts, $schemaID, $this->form); | ||
| $response = HTTPResponse::create(json_encode($data)); | ||
| $response->addHeader('Content-Type', 'application/json'); | ||
| return $response; | ||
| } |
There was a problem hiding this comment.
Works very similar to FormSchemaController in admin, but is needed in framework so GridField can call it.
I looked at whether FormSchemaController can rely on this but it would require refactoring beyond what I'm willing to do in this set of PRs.
There was a problem hiding this comment.
Maybe the bulk of this could be moved into FormSchema and then called from both here and FormSchemaController to reduce a little bit of code duplication - let me know if you want me to explore that. Wouldn't be much effort (if it's doable in a sensible way), but also low value.
There was a problem hiding this comment.
I wouldn't worry above it, the fact that FormSchema::singleton()->getMultipartSchema() exists is really the bit that reduces code duplication.
| * @param GridField $gridfield | ||
| * @return string | ||
| */ | ||
| public function getSearchFieldSchema(GridField $gridField) |
There was a problem hiding this comment.
This was largely duplicated so has been moved into SearchContextForm
| * @param GridField $gridfield | ||
| * @return HTTPResponse | ||
| */ | ||
| public function getSearchFormSchema(GridField $gridField) |
There was a problem hiding this comment.
This was largely duplicated so has been moved into SearchContextFormFormRequestHandler
| * If getPlaceHolderText is empty, this object will be used to build the placeholder | ||
| * e.g. 'Search "My Data Object"' | ||
| */ | ||
| private function getPlaceHolder(object $obj): string |
There was a problem hiding this comment.
Moved into the new SearchContextForm which has some special handling for getPlaceholderText()
There was a problem hiding this comment.
Private so doesn't need to be deprecated
| /* | ||
| * Append a prefix to search field names to prevent conflicts with other fields in the search form | ||
| */ | ||
| private function addSearchPrefixToFields(FieldList $fields): void |
There was a problem hiding this comment.
Handled by the new form instead
There was a problem hiding this comment.
Private so doesn't need to be deprecated
| /** | ||
| * Update CSS classes for form fields, including nested inside composite fields | ||
| */ | ||
| private function updateFieldClasses(FieldList $fields): void |
There was a problem hiding this comment.
Handled by the new form instead
There was a problem hiding this comment.
Private so doesn't need to be deprecated
| return $this; | ||
| } | ||
|
|
||
| public function __clone(): void |
There was a problem hiding this comment.
See usage in SearchContextForm - also just seems sensible, given FieldList already handles cloning.
| } | ||
| } | ||
|
|
||
| private function removeSearchPrefixFromFields(FieldList $fields): void |
There was a problem hiding this comment.
Private so doesn't need to be deprecated
There was a problem hiding this comment.
Changes here are to accomodate cloning
| $this->setValidator(clone $this->validator); | ||
| } | ||
| foreach ($this->fields as $field) { | ||
| $field->setForm($this); |
There was a problem hiding this comment.
Is this recursive? e.g. will field groups and composite fields ensure their descendants call $field->setForm() properly?
I'd kind of expect this to happen in the FieldList cloning you mentioned in the comment above, so I'm a little surprised to see it here
There was a problem hiding this comment.
FieldList does have some logic, CompositeField also has some logic. But the form being cloned is where the form should be set. Cloning a FieldList doesn't mean the form is changing. The FieldList doesn't even keep track of the form, for that matter.
| /** | ||
| * Gets a JSON schema representing a form. | ||
| */ | ||
| public function getSchema(HTTPRequest $request): HTTPResponse | ||
| { | ||
| $schemaID = $request->getURL(); | ||
| $parts = $request->getHeader(FormSchema::SCHEMA_HEADER); | ||
| $data = FormSchema::singleton()->getMultipartSchema($parts, $schemaID, $this->form); | ||
| $response = HTTPResponse::create(json_encode($data)); | ||
| $response->addHeader('Content-Type', 'application/json'); | ||
| return $response; | ||
| } |
There was a problem hiding this comment.
I wouldn't worry above it, the fact that FormSchema::singleton()->getMultipartSchema() exists is really the bit that reduces code duplication.
| /** | ||
| * A form for using a search context in the CMS. | ||
| * | ||
| * Note that the form submission goes through to the index action of the controller by default, |
There was a problem hiding this comment.
| * Note that the form submission goes through to the index action of the controller by default, | |
| * Note that the form submission goes through to the index action of the controller by default, |
Could you point me to the bit where the controller actually processes the search form submission, I'm not sure where it is
There was a problem hiding this comment.
See CMSMain::ListViewForm() and GridFieldFilterHeader::getManipulatedData()
| $this->prepareSearchFieldsForCMS($searchFields); | ||
| parent::__construct($controller, $name, $searchFields); | ||
| $this->setHTMLID(ClassInfo::shortName($controller) . '_' . $name); | ||
| $this->disableSecurityToken(); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Cool, could you add that comment back in above it so others know why it's done
| public function __construct(RequestHandler $controller, SearchContext $searchContext, $name = 'SearchForm') | ||
| { | ||
| $this->searchContext = $searchContext; | ||
| $searchFields = clone $searchContext->getSearchFields(); |
There was a problem hiding this comment.
Why do we need to clone the search fields here?
There was a problem hiding this comment.
Because otherwise prepareSearchFieldsForCMS() will alter the names of the fields in place, which will affect the functionality within the search context itself that relies on form fields with specific names.
There was a problem hiding this comment.
OK, just add that in an inline comment above so future generation know why it's cloned
src/ORM/Search/SearchContextForm.php
Outdated
| $this->removeSearchPrefixFromFields($form->Fields()); | ||
| $form->loadDataFrom($values); | ||
|
|
||
| // Get the values from the form where possible, as form fields may transform the value for DB coparisons |
There was a problem hiding this comment.
| // Get the values from the form where possible, as form fields may transform the value for DB coparisons | |
| // Get the values from the form where possible, as form fields may transform the value for DB comparisons |
| } | ||
|
|
||
| /** | ||
| * Get the schema for this search context for use in the CMS search filter form. |
There was a problem hiding this comment.
This is really confusing having 2x schemas. Is there another way we could refer to this?
There was a problem hiding this comment.
This paradigm already exists, and they are both schemas... I can't think of another way to refer to it and don't really want to be reinventing things this close to the beta release.
src/ORM/Search/SearchContextForm.php
Outdated
| */ | ||
| public function getSchemaData(): array | ||
| { | ||
| $schemaUrl = $this->getRequestHandler()->Link('schema'); |
There was a problem hiding this comment.
This could be called $formSchemaUrl instead? OK to keep as $schemaUrl if we no longer call the method getSchemaData() i.e. remove the word 'Schema' from it
There was a problem hiding this comment.
Updated the variable name
GuySartorelli
force-pushed
the
pulls/6.0/refactor-sitetree-filter
branch
from
613653c to
160d5f6
Compare
| public function __construct(RequestHandler $controller, SearchContext $searchContext, $name = 'SearchForm') | ||
| { | ||
| $this->searchContext = $searchContext; | ||
| $searchFields = clone $searchContext->getSearchFields(); |
There was a problem hiding this comment.
OK, just add that in an inline comment above so future generation know why it's cloned
| $this->prepareSearchFieldsForCMS($searchFields); | ||
| parent::__construct($controller, $name, $searchFields); | ||
| $this->setHTMLID(ClassInfo::shortName($controller) . '_' . $name); | ||
| $this->disableSecurityToken(); |
There was a problem hiding this comment.
Cool, could you add that comment back in above it so others know why it's done
GuySartorelli
force-pushed
the
pulls/6.0/refactor-sitetree-filter
branch
from
160d5f6 to
913c74e
Compare
CMSMain had different search filter code paths than the grid field search filter, and there was a lot of duplication of code. This commit works alongside a commit in silverstripe/silverstripe-cms to unify those code paths
GuySartorelli
force-pushed
the
pulls/6.0/refactor-sitetree-filter
branch
from
913c74e to
35c5f26
Compare
|
Minor changes to |
2 participants
CMSMain had different search filter code paths than the grid field search filter, and there was a lot of duplication of code. This commit works alongside the PR in silverstripe/silverstripe-cms to unify those code paths
Issue
CMSMain/HierarchyModelAdminsilverstripe-cms#2949