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

ENH Improve type safety to support refactored template layer #3010

Merged
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
24 changes: 7 additions & 17 deletions code/Controllers/ContentController.php
Original file line number Diff line number Diff line change
Expand Up @@ -290,16 +290,6 @@ public function getMenu($level = 1)
return new ArrayList($visible);
}

/**
* @return ArrayList<SiteTree>
* @deprecated 5.4.0 Use getMenu() instead. You can continue to use $Menu in templates.
*/
public function Menu($level)
{
Deprecation::noticeWithNoReplacment('5.4.0', 'Use getMenu() instead. You can continue to use $Menu in templates.');
return $this->getMenu($level);
}
Comment on lines -293 to -301
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't needed anymore. It existed because you couldn't pass args into getters from a template using $Menu(2) syntax.
I've resolved that - the args now get passed through to the getter.


/**
* Returns the default log-in form.
*
Expand Down Expand Up @@ -416,23 +406,23 @@ public function getViewer($action)
$action = $action === 'index' ? '' : '_' . $action;

$templatesFound = [];
// Find templates for the record + action together - e.g. Page_action.ss
// Find templates for the record + action together - e.g. Page_action
if ($this->dataRecord instanceof SiteTree) {
$templatesFound[] = $this->dataRecord->getViewerTemplates($action);
}

// Find templates for the controller + action together - e.g. PageController_action.ss
$templatesFound[] = SSViewer::get_templates_by_class(static::class, $action, Controller::class);
// Find templates for the controller + action together - e.g. PageController_action
$templatesFound[] = SSViewer::get_templates_by_class(static::class, $action ?? '', Controller::class);
Copy link
Member Author

Choose a reason for hiding this comment

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

$action isn't strongly typed so I've added a little type safety here.


// Find templates for the record without an action - e.g. Page.ss
// Find templates for the record without an action - e.g. Page
if ($this->dataRecord instanceof SiteTree) {
$templatesFound[] = $this->dataRecord->getViewerTemplates();
}

// Find the templates for the controller without an action - e.g. PageController.ss
$templatesFound[] = SSViewer::get_templates_by_class(static::class, "", Controller::class);
// Find the templates for the controller without an action - e.g. PageController
$templatesFound[] = SSViewer::get_templates_by_class(static::class, '', Controller::class);

$templates = array_merge(...$templatesFound);
return SSViewer::create($templates);
return SSViewer::create($templates, $this->getTemplateEngine());
Copy link
Member Author

Choose a reason for hiding this comment

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

See Controller::getTemplateEngine() in the framework PR

}
}
4 changes: 2 additions & 2 deletions code/Model/SiteTree.php
Original file line number Diff line number Diff line change
Expand Up @@ -1454,7 +1454,7 @@ public function MetaComponents()

$tags['title'] = [
'tag' => 'title',
'content' => $this->obj('Title')->forTemplate()
'content' => $this->obj('Title')?->forTemplate()
Copy link
Member Author

Choose a reason for hiding this comment

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

obj() can return null

];

$generator = $this->getGenerator();
Expand Down Expand Up @@ -1601,7 +1601,7 @@ public function MetaTags($includeTitle = true)

$tagString = implode("\n", $tags);
if ($this->ExtraMeta) {
$tagString .= $this->obj('ExtraMeta')->forTemplate();
$tagString .= $this->obj('ExtraMeta')?->forTemplate();
}

$this->extend('updateMetaTags', $tagString);
Expand Down
2 changes: 1 addition & 1 deletion tests/behat/src/ThemeContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public function stepCreateTheme($theme)
}

/**
* Create a template within a test theme
* Create a template within a test theme. Only ss templates are supported.
*
* @Given /^a template "(?<template>[^"]+)" in theme "(?<theme>[^"]+)" with content "(?<content>[^"]+)"/
* @param string $template
Expand Down
31 changes: 20 additions & 11 deletions tests/php/Controllers/ContentControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
use SilverStripe\CMS\Controllers\ContentController;
use SilverStripe\CMS\Controllers\RootURLController;
use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\CMS\Tests\Controllers\ContentControllerTest\DummyTemplateContentController;
use SilverStripe\CMS\Tests\Controllers\ContentControllerTest\DummyTemplateEngine;
use SilverStripe\Control\Director;
use SilverStripe\Control\HTTPResponse_Exception;
use SilverStripe\Core\Config\Config;
Expand Down Expand Up @@ -162,7 +164,7 @@ public function testGetViewer()
{
$this->useTestTheme(__DIR__, 'controllertest', function () {

// Test a page without a controller (ContentControllerTest_PageWithoutController.ss)
// Test a page without a controller (ContentControllerTest_PageWithoutController)
$page = new ContentControllerTestPageWithoutController();
$page->URLSegment = "test";
$page->write();
Expand All @@ -171,7 +173,7 @@ public function testGetViewer()
$response = $this->get($page->RelativeLink());
$this->assertEquals("ContentControllerTestPageWithoutController", trim($response->getBody() ?? ''));

// This should fall over to user Page.ss
// This should fall over to use Page
$page = new ContentControllerTestPage();
$page->URLSegment = "test";
$page->write();
Expand All @@ -191,20 +193,27 @@ public function testGetViewer()
$this->assertEquals("ContentControllerTestPage_test", trim($response->getBody() ?? ''));

// Test that an action without a template will default to the index template, which is
// to say the default Page.ss template
// to say the default Page template
$response = $this->get($page->RelativeLink("testwithouttemplate"));
$this->assertEquals("Page", trim($response->getBody() ?? ''));

// Test that an action with a template will render the both action template *and* the
// Test that an action with a template will render both the action template *and* the
// correct parent template
$controller = new ContentController($page);
$controller = new DummyTemplateContentController($page);
$viewer = $controller->getViewer('test');
$this->assertEquals(
__DIR__
. '/themes/controllertest/templates/SilverStripe/CMS/Tests/Controllers/'
. 'ContentControllerTestPage_test.ss',
$viewer->templates()['main']
);
/** @var DummyTemplateEngine $engine */
$engine = $viewer->getTemplateEngine();
$templateCandidates = $engine->getTemplates();
// Check for the action templates
$this->assertContains('SilverStripe\CMS\Tests\Controllers\ContentControllerTestPage_test', $templateCandidates);
$this->assertContains('SilverStripe\CMS\Model\SiteTree_test', $templateCandidates);
$this->assertContains('SilverStripe\CMS\Tests\Controllers\ContentControllerTest\DummyTemplateContentController_test', $templateCandidates);
$this->assertContains('SilverStripe\CMS\Controllers\ContentController_test', $templateCandidates);
// Check for the parent templates
$this->assertContains('SilverStripe\CMS\Tests\Controllers\ContentControllerTestPage', $templateCandidates);
$this->assertContains('SilverStripe\CMS\Model\SiteTree', $templateCandidates);
$this->assertContains('SilverStripe\CMS\Tests\Controllers\ContentControllerTest\DummyTemplateContentController', $templateCandidates);
$this->assertContains('SilverStripe\CMS\Controllers\ContentController', $templateCandidates);
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

namespace SilverStripe\CMS\Tests\Controllers\ContentControllerTest;

use SilverStripe\CMS\Controllers\ContentController;
use SilverStripe\Dev\TestOnly;
use SilverStripe\View\TemplateEngine;

class DummyTemplateContentController extends ContentController implements TestOnly
{
protected function getTemplateEngine(): TemplateEngine
{
return new DummyTemplateEngine();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php

namespace SilverStripe\CMS\Tests\Controllers\ContentControllerTest;

use SilverStripe\Dev\TestOnly;
use SilverStripe\View\TemplateEngine;
use SilverStripe\View\ViewLayerData;

/**
* A dummy template renderer that doesn't actually render any templates.
*/
class DummyTemplateEngine implements TemplateEngine, TestOnly
{
private string $output = '<html><head></head><body></body></html>';

private string|array $templates;

public function __construct(string|array $templateCandidates = [])
{
$this->templates = $templateCandidates;
}

public function setTemplate(string|array $templateCandidates): static
{
$this->templates = $templateCandidates;
return $this;
}

public function hasTemplate(string|array $templateCandidates): bool
{
return true;
}

public function renderString(string $template, ViewLayerData $model, array $overlay = [], bool $cache = true): string
{
return $this->output;
}

public function render(ViewLayerData $model, array $overlay = []): string
{
return $this->output;
}

public function setOutput(string $output): void
{
$this->output = $output;
}

/**
* Returns the template candidates that were passed to the constructor or to setTemplate()
*/
public function getTemplates(): array
{
return $this->templates;
}
}
Loading