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.

Sign up for GitHub

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

API Make ElementalAreaController a subclass of FormSchemaController #1273

Merged
merged 1 commit into from
Nov 19, 2024
Merged

API Make ElementalAreaController a subclass of FormSchemaController #1273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions src/Controllers/ElementalAreaController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
use DNADesign\Elemental\Forms\EditFormFactory;
use DNADesign\Elemental\Models\BaseElement;
use DNADesign\Elemental\Services\ElementTypeRegistry;
use SilverStripe\CMS\Controllers\CMSMain;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Forms\Form;
Expand All @@ -20,17 +19,18 @@
use Exception;
use SilverStripe\Control\HTTPRequest;
use InvalidArgumentException;
use SilverStripe\Admin\FormSchemaController;

/**
* Controller for "ElementalArea" - handles loading and saving of in-line edit forms in an elemental area in admin
*/
class ElementalAreaController extends CMSMain
class ElementalAreaController extends FormSchemaController
Copy link
Member Author

@GuySartorelli GuySartorelli Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This was originally changed from subclassing LeftAndMain to CMSMain in #449

The reasoning was

It makes more sense for ElementalAreaController to inherit from CMSMain rather than LeftAndMain, as this means that permissions are checked in relation to the Page Level rather than for the entire CMS.

Maybe that made sense at the time, but you can have an elemental area on any DataObject including inside a ModelAdmin.... so requiring CMS_ACCESS_CMSMain permissions is just wrong. Instead, require CMS access more generally, and let the model permissions dictate what can and can't be done within the controller.

{
const FORM_NAME_TEMPLATE = 'ElementForm_%s';

private static $url_segment = 'elemental-area';

private static $ignore_menuitem = true;
private static string $required_permission_codes = 'CMS_ACCESS';

private static $url_handlers = [
'elementForm/$ItemID' => 'elementForm',
Expand Down
8 changes: 4 additions & 4 deletions tests/Controllers/ElementalAreaControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
class ElementalAreaControllerTest extends FunctionalTest
{
protected static $fixture_file = 'ElementalAreaControllerTest.yml';

protected static $extra_dataobjects = [
TestElementContent::class,
TestElementalArea::class,
Expand Down Expand Up @@ -180,9 +180,9 @@ public function testElementFormPost(string $idType, string $dataType, string $fa
$response = $this->post($url, $data, $headers);
$this->assertSame($expectedCode, $response->getStatusCode());
if ($fail === 'csrf-token') {
// Will end up at an HTML page with "Silverstripe - Bad Request"
$this->assertSame('text/html; charset=utf-8', $response->getHeader('Content-type'));
$this->assertStringContainsString('Silverstripe - Bad Request', $response->getBody());
// Gives suitable error message for XHR request
$this->assertStringStartsWith('text/plain', $response->getHeader('Content-type'));
$this->assertStringContainsString('There seems to have been a technical problem', $response->getBody());
Comment on lines -183 to +185
Copy link
Member Author

Choose a reason for hiding this comment

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

The modal links used to be routed through LeftAndMain which meant the error handling in that class was used. For some reason they weren't picked up as XHR requests so the full HTML error page was being rendered which wasn't really the correct behaviour, but it's what happened.

Now, a simple 400 response (the default one produced when a form's CSRF token is missing) is produced and isn't altered. This is the correct behaviour.

The end result for the user is no change.

return;
}
$this->assertSame($expectedCode, $response->getStatusCode());
Expand Down
Loading