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

API Make SubsiteXHRController a subclass of AdminController #611

Merged
merged 2 commits into from
Nov 19, 2024
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
2 changes: 1 addition & 1 deletion client/dist/js/LeftAndMain_Subsites.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 8 additions & 14 deletions client/src/js/LeftAndMain_Subsites.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,6 @@
}
});

/*
* Reload subsites dropdown when links are processed
*/
$('.cms-container .cms-menu-list li a').entwine({
Copy link
Member Author

Choose a reason for hiding this comment

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

Not only is this selector wrong (should be .cms-menu__list) but this just straight up isn't necessary. Clicking on the links in the left menu reloads the whole left menu anyway.

onclick(e) {
$('.cms-container').loadFragment('admin/subsite_xhr', 'SubsiteList');
this._super(e);
}
});

/*
* Reload subsites dropdown when the admin area reloads (for deleting sites)
*/
Expand All @@ -32,11 +22,15 @@
});

/*
* Reload subsites dropdown when subsites are added or names are modified
* Reload subsites dropdown when subsites are added or modified
*/
$('.cms-container .tab.subsite-model').entwine({
onadd(e) {
$('.cms-container').loadFragment('admin/subsite_xhr', 'SubsiteList');
$('#Form_ItemEditForm').entwine({
onaftersubmitform(e) {
// Only load the fragment if this form is the subsite form.
// We can't add a selector to the form itself so check for the tab we added a specific class to.
if (this.find('[role="tab"].subsite-model').length > 0) {
$('.cms-container').loadFragment('admin/subsite_xhr', 'SubsiteList');
}
this._super(e);
}
Comment on lines -37 to 35
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was trying to reload the selector any time it saw the root.main tab in the subsite edit form.
The selector was wrong, but the approach was wrong too - we should only reload the selector after the form is submitted, not just any time the form loads.

});
Expand Down
46 changes: 19 additions & 27 deletions src/Controller/SubsiteXHRController.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,27 @@

namespace SilverStripe\Subsites\Controller;

use SilverStripe\Admin\LeftAndMain;
use SilverStripe\Admin\AdminController;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Control\PjaxResponseNegotiator;
use SilverStripe\Dev\Deprecation;
use SilverStripe\ORM\FieldType\DBHTMLText;
use SilverStripe\Security\Member;
use SilverStripe\Security\Permission;
use SilverStripe\Subsites\Model\Subsite;

/**
* Section-agnostic PJAX controller.
* Section-agnostic PJAX controller that renders the subsites swapper dropdown
*/
class SubsiteXHRController extends LeftAndMain
class SubsiteXHRController extends AdminController
{
private static $url_segment = 'subsite_xhr';

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

public function index(HTTPRequest $request): HTTPResponse
{
return $this->getResponseNegotiator()->respond($request);
}
Comment on lines +22 to +25
Copy link
Member Author

@GuySartorelli GuySartorelli Nov 12, 2024

Choose a reason for hiding this comment

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

This is necessary because we're using loadFragment in js rather than directly hitting the SubsiteList method as a controller action


/**
* Relax the access permissions, so anyone who has access to any CMS subsite can access this controller.
Expand All @@ -37,35 +43,21 @@ public function canView($member = null)
}

/**
* Allow access if user allowed into the CMS at all.
* @deprecated 3.4.0 Will be removed without equivalent functionality to replace it.
* Get a Pjax response negotiator for the subsite list
*/
public function canAccess()
Copy link
Member Author

Choose a reason for hiding this comment

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

canAccess() is a non-standard method that's only applied to LeftAndMain classes via the LeftAndMainSubsites extension. It no longer applies here, so we use required_permission_codes instead set to the less restrictive of these since canView() is handling the main access check for us.

{
Deprecation::noticeWithNoReplacment('3.4.0');
// Allow if any cms access is available
return Permission::check([
'CMS_ACCESS', // Supported by 3.1.14 and up
'CMS_ACCESS_LeftAndMain'
]);
}

public function getResponseNegotiator(): PjaxResponseNegotiator
{
$negotiator = parent::getResponseNegotiator();

// Register a new callback
$negotiator->setCallback('SubsiteList', function () {
return $this->SubsiteList();
});

return $negotiator;
return new PjaxResponseNegotiator([
'SubsiteList' => function () {
return $this->SubsiteList();
},
]);
Comment on lines +50 to +54
Copy link
Member Author

@GuySartorelli GuySartorelli Nov 12, 2024

Choose a reason for hiding this comment

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

That's the only fragment this controller ever cares about.
Using new because that's what's done everywhere that uses it right now and it's not an Injectable class.

}

/**
* Provide the list of available subsites as a cms-section-agnostic PJAX handler.
*/
public function SubsiteList()
public function SubsiteList(): DBHTMLText
{
return $this->renderWith(['type' => 'Includes', SubsiteXHRController::class . '_subsitelist']);
}
Expand Down
15 changes: 0 additions & 15 deletions src/Extensions/LeftAndMainSubsites.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Convert;
use SilverStripe\Core\Extension;
use SilverStripe\Dev\Deprecation;
use SilverStripe\Forms\HiddenField;
use SilverStripe\Model\List\ArrayList;
use SilverStripe\ORM\DataObject;
Expand Down Expand Up @@ -69,8 +68,6 @@ public static function SubsiteSwitchList(): SS_List
return false;
}

Requirements::javascript('silverstripe/subsites:client/dist/js/LeftAndMain_Subsites.js');

$output = ArrayList::create();

foreach ($list as $subsite) {
Expand Down Expand Up @@ -180,18 +177,6 @@ public function Subsites()
return Subsite::all_accessible_sites();
}

/**
* Generates a list of subsites with the data needed to
* produce a dropdown site switcher
* @return ArrayList<Subsite>
* @deprecated 3.4.0 Will be removed without equivalent functionality to replace it.
*/
public function ListSubsites()
{
Deprecation::notice('3.4.0', 'Use SubsiteSwitchList() instead.');
return static::SubsiteSwitchList();
}

public function alternateMenuDisplayCheck($controllerName)
{
if (!class_exists($controllerName ?? '')) {
Expand Down
1 change: 1 addition & 0 deletions templates/SilverStripe/Admin/LeftAndMain_Menu.ss
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
<% include SilverStripe\\Admin\\LeftAndMain_MenuStatus %>

<% if $SubsiteSwitchList.Count > 1 %>
<% require javascript('silverstripe/subsites:client/dist/js/LeftAndMain_Subsites.js') %>
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the require code here instead of in the method since we don't need to re-require it for the xhr requests and because frankly a require call in an arbitrary PHP method is just gross

<% include SilverStripe\\Subsites\\Controller\\SubsiteXHRController_subsitelist %>
<% end_if %>
</div>
Expand Down