Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/Forms/Form.php
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,9 @@ public function __construct(
}

// Form error controls
$this->restoreFormState();
if ($controller) {
$this->restoreFormState();
}
Comment on lines +311 to +313
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

$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.


// Check if CSRF protection is enabled, either on the parent controller or from the default setting. Note that
// method_exists() is used as some controllers (e.g. GroupTest) do not always extend from Object.
Expand Down Expand Up @@ -1790,6 +1792,21 @@ public function removeExtraClass($class)
return $this;
}

public function __clone(): void
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

See usage in SearchContextForm - also just seems sensible, given FieldList already handles cloning.

{
$this->fields = clone $this->fields;
$this->actions = clone $this->actions;
if ($this->validator) {
$this->setValidator(clone $this->validator);
}
foreach ($this->fields as $field) {
$field->setForm($this);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@GuySartorelli GuySartorelli Mar 3, 2025

Choose a reason for hiding this comment

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

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.

}
foreach ($this->actions as $action) {
$action->setForm($this);
}
}

public function debug(): string
{
$class = static::class;
Expand Down
17 changes: 15 additions & 2 deletions src/Forms/FormRequestHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,15 @@

use BadMethodCallException;
use SilverStripe\Control\Controller;
use SilverStripe\Control\Director;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Control\HTTPResponse_Exception;
use SilverStripe\Control\RequestHandler;
use SilverStripe\Core\ClassInfo;
use SilverStripe\Core\Convert;
use SilverStripe\Core\Validation\ValidationResult;
use SilverStripe\Model\List\SS_List;
use SilverStripe\Core\Validation\ValidationException;
use SilverStripe\Forms\Schema\FormSchema;

class FormRequestHandler extends RequestHandler
{
Expand All @@ -27,6 +26,7 @@ class FormRequestHandler extends RequestHandler
* @var array
*/
private static $allowed_actions = [
'getSchema',
'handleField',
'httpSubmission',
'forTemplate',
Expand All @@ -37,6 +37,7 @@ class FormRequestHandler extends RequestHandler
* @var array
*/
private static $url_handlers = [
'GET schema' => 'getSchema',
'field/$FieldName!' => 'handleField',
'POST ' => 'httpSubmission',
'GET ' => 'httpSubmission',
Expand Down Expand Up @@ -67,6 +68,18 @@ public function __construct(Form $form)
}
}

/**
* 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;
}
Comment on lines +71 to +82
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@emteknetnz emteknetnz Mar 3, 2025

Choose a reason for hiding this comment

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

I wouldn't worry above it, the fact that FormSchema::singleton()->getMultipartSchema() exists is really the bit that reduces code duplication.


/**
* Get link for this form
Expand Down
11 changes: 8 additions & 3 deletions src/Forms/GridField/GridField.php
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,8 @@ public function performReadonlyTransformation()
$hadEditButton = $copyConfig->getComponentByType(GridFieldEditButton::class) !== null;

// get the whitelist for allowable readonly components
$allowedComponents = $this->getReadonlyComponents();
foreach ($this->getConfig()->getComponents() as $component) {
$allowedComponents = $copy->getReadonlyComponents();
foreach ($copy->getConfig()->getComponents() as $component) {
// if a component doesn't exist, remove it from the readonly version.
if (!in_array(get_class($component), $allowedComponents ?? [])) {
$copyConfig->removeComponent($component);
Expand All @@ -297,7 +297,7 @@ public function performReadonlyTransformation()
}
}

$copy->extend('afterPerformReadonlyTransformation', $this);
$copy->extend('afterPerformReadonlyTransformation', $copy);

return $copy;
}
Expand Down Expand Up @@ -1346,6 +1346,11 @@ public function saveInto(DataObjectInterface $record)
}
}

public function __clone(): void
{
$this->config = clone $this->config;
}

/**
* @param array $content
*
Expand Down
9 changes: 9 additions & 0 deletions src/Forms/GridField/GridFieldConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -155,4 +155,13 @@ public function getComponentByType($type)
}
return null;
}

public function __clone(): void
{
$components = $this->components;
$this->components = ArrayList::create();
foreach ($components as $component) {
$this->components->add(clone $component);
}
}
}
Loading