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 Deprecations for template layer #11420

Merged
merged 1 commit into from
Oct 21, 2024
Merged

API Deprecations for template layer #11420

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
3 changes: 3 additions & 0 deletions src/Forms/GridField/GridFieldDataColumns.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use SilverStripe\Core\Convert;
use InvalidArgumentException;
use LogicException;
use SilverStripe\Dev\Deprecation;
use SilverStripe\View\ViewableData;

/**
Expand Down Expand Up @@ -228,9 +229,11 @@ public function getColumnMetadata($gridField, $column)
* @param ViewableData $record
* @param string $columnName
* @return string|null - returns null if it could not found a value
* @deprecated 5.4.0 Will be removed without equivalent functionality to replace it.
*/
protected function getValueFromRelation($record, $columnName)
Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out this method just isn't used at all. It used to be used in getColumnContent() but the work this method used to do is now offloaded to GridField::getDataFieldValue().

Found this because it's the only usage of XML_val() in core.

{
Deprecation::notice('5.4.0', 'Will be removed without equivalent functionality to replace it.');
$fieldNameParts = explode('.', $columnName ?? '');
$tmpItem = clone($record);
for ($idx = 0; $idx < sizeof($fieldNameParts ?? []); $idx++) {
Expand Down
2 changes: 1 addition & 1 deletion src/View/SSTemplateParser.peg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,7 @@ class SSTemplateParser extends Parser implements TemplateParser
$arguments = $res['arguments'];

// Note: 'type' here is important to disable subTemplates in SSViewer::getSubtemplateFor()
$res['php'] = '$val .= \\SilverStripe\\View\\SSViewer::execute_template([["type" => "Includes", '.$template.'], '.$template.'], $scope->getItem(), [' .
$res['php'] = '$val .= \\SilverStripe\\View\\SSViewer::execute_template([["type" => "Includes", '.$template.'], '.$template.'], $scope->getCurrentItem(), [' .
implode(',', $arguments)."], \$scope, true);\n";

if ($this->includeDebuggingComments) { // Add include filename comments on dev sites
Expand Down
2 changes: 1 addition & 1 deletion src/View/SSTemplateParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3897,7 +3897,7 @@ function Include__finalise(&$res)
$arguments = $res['arguments'];

// Note: 'type' here is important to disable subTemplates in SSViewer::getSubtemplateFor()
$res['php'] = '$val .= \\SilverStripe\\View\\SSViewer::execute_template([["type" => "Includes", '.$template.'], '.$template.'], $scope->getItem(), [' .
$res['php'] = '$val .= \\SilverStripe\\View\\SSViewer::execute_template([["type" => "Includes", '.$template.'], '.$template.'], $scope->getCurrentItem(), [' .
implode(',', $arguments)."], \$scope, true);\n";

if ($this->includeDebuggingComments) { // Add include filename comments on dev sites
Expand Down
81 changes: 75 additions & 6 deletions src/View/SSViewer.php
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion src/View/SSViewer_DataPresenter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@

use InvalidArgumentException;
use SilverStripe\Core\ClassInfo;
use SilverStripe\Dev\Deprecation;
use SilverStripe\ORM\FieldType\DBField;

/**
* This extends SSViewer_Scope to mix in data on top of what the item provides. This can be "global"
* data that is scope-independant (like BaseURL), or type-specific data that is layered on top cross-cut like
* (like $FirstLast etc).
*
* It's separate from SSViewer_Scope to keep that fairly complex code as clean as possible.
* @deprecated 5.4.0 Will be merged into SilverStripe\View\SSViewer_Scope
*/
class SSViewer_DataPresenter extends SSViewer_Scope
Comment on lines -14 to 17
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 know this was intentionally kept separate, but whatever the case was when that decision was made it was causing me headaches trying to sort out what the difference is between SSViewer_Scope and this class. Especially since they have a lot of similar logic in them - the __call() class is a great example where the code flow is not obvious with this class split out like this.

I've opted to merge the two classes together, and simplify where I can. There are or will be additional cards to simplify it further in the future, too.

{
Expand Down Expand Up @@ -65,6 +66,7 @@ public function __construct(
array $underlay = null,
SSViewer_Scope $inheritedScope = null
) {
Deprecation::noticeWithNoReplacment('5.4.0', 'Will be merged into ' . SSViewer_Scope::class, Deprecation::SCOPE_CLASS);
parent::__construct($item, $inheritedScope);

$this->overlay = $overlay ?: [];
Expand Down
7 changes: 7 additions & 0 deletions src/View/SSViewer_FromString.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
namespace SilverStripe\View;

use SilverStripe\Core\Config\Config;
use SilverStripe\Dev\Deprecation;

/**
* Special SSViewer that will process a template passed as a string, rather than a filename.
* @deprecated 5.4.0 Will be replaced with SilverStripe\View\SSTemplateEngine::renderString()
*/
class SSViewer_FromString extends SSViewer
Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't make sense to have this abstraction anymore - if you have a template string, it'll be for a specific template syntax so you should make sure to get the correct template engine from the injector and ask it to render your template string directly.

{
Expand Down Expand Up @@ -37,6 +39,11 @@ class SSViewer_FromString extends SSViewer
*/
public function __construct($content, TemplateParser $parser = null)
{
Deprecation::noticeWithNoReplacment(
'5.4.0',
'Will be replaced with SilverStripe\View\SSTemplateEngine::renderString()',
Deprecation::SCOPE_CLASS
);
if ($parser) {
$this->setParser($parser);
}
Expand Down
22 changes: 18 additions & 4 deletions src/View/SSViewer_Scope.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use ArrayIterator;
use Countable;
use Iterator;
use SilverStripe\Dev\Deprecation;
use SilverStripe\ORM\FieldType\DBBoolean;
use SilverStripe\ORM\FieldType\DBText;
use SilverStripe\ORM\FieldType\DBFloat;
Expand Down Expand Up @@ -123,16 +124,23 @@ public function __construct($item, SSViewer_Scope $inheritedScope = null)
* Returns the current "active" item
*
* @return object
* @deprecated 5.4.0 use getCurrentItem() instead.
*/
public function getItem()
{
Deprecation::notice('5.4.0', 'use getCurrentItem() instead.');
Comment on lines +127 to +131
Copy link
Member Author

Choose a reason for hiding this comment

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

Just renamed it to be a little clearer that it's not just getting the value of the item property. I ran into problems when I tried to replace all instances of $this->item with calling this method, because sometimes that's not what this method returns.

$item = $this->itemIterator ? $this->itemIterator->current() : $this->item;
if (is_scalar($item)) {
$item = $this->convertScalarToDBField($item);
}
return $item;
}

public function getCurrentItem()
{
return $this->getItem();
}

/**
* Called at the start of every lookup chain by SSTemplateParser to indicate a new lookup from local scope
*
Expand Down Expand Up @@ -185,7 +193,7 @@ public function resetLocalScope()
*/
public function getObj($name, $arguments = [], $cache = false, $cacheName = null)
{
$on = $this->getItem();
$on = $this->getCurrentItem();
if ($on === null) {
return null;
}
Expand All @@ -198,9 +206,11 @@ public function getObj($name, $arguments = [], $cache = false, $cacheName = null
* @param bool $cache
* @param string $cacheName
* @return $this
* @deprecated 5.4.0 Will be renamed scopeToIntermediateValue()
*/
public function obj($name, $arguments = [], $cache = false, $cacheName = null)
Comment on lines +209 to 211
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 is getting renamed so it's clear what it's for

{
Deprecation::noticeWithNoReplacment('5.4.0', 'Will be renamed scopeToIntermediateValue()');
switch ($name) {
case 'Up':
if ($this->upIndex === null) {
Expand Down Expand Up @@ -252,7 +262,7 @@ public function obj($name, $arguments = [], $cache = false, $cacheName = null)
*/
public function self()
{
$result = $this->getItem();
$result = $this->getCurrentItem();
$this->resetLocalScope();

return $result;
Expand Down Expand Up @@ -350,8 +360,12 @@ public function next()
*/
public function __call($name, $arguments)
{
$on = $this->getItem();
$retval = $on ? $on->$name(...$arguments) : null;
$on = $this->getCurrentItem();
if ($on instanceof ViewableData && $name === 'XML_val') {
$retval = $on->XML_val(...$arguments);
} else {
$retval = $on ? $on->$name(...$arguments) : null;
}

$this->resetLocalScope();
return $retval;
Expand Down
3 changes: 3 additions & 0 deletions src/View/ThemeResourceLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use SilverStripe\Core\Manifest\ModuleLoader;
use SilverStripe\Core\Manifest\ModuleResourceLoader;
use SilverStripe\Core\Path;
use SilverStripe\Dev\Deprecation;

/**
* Handles finding templates from a stack of template manifest objects.
Expand Down Expand Up @@ -182,9 +183,11 @@ public function getPath($identifier)
* @return string Absolute path to resolved template file, or null if not resolved.
* File location will be in the format themes/<theme>/templates/<directories>/<type>/<basename>.ss
* Note that type (e.g. 'Layout') is not the root level directory under 'templates'.
* @deprecated 5.4.0 Will be removed without equivalent functionality to replace it.
*/
public function findTemplate($template, $themes = null)
Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't make sense for this to be here anymore, since this only knows about ss templates.

There is a replacement method for this in the new engine but I've opted to keep it private, to dissuade people from trying to deal with file paths for templates - we should be thinking and talking about template files as template "candidates" instead, i.e. "render with Includes/Header" and let the template engine figure out through theme inheritance which exact template file that maps to.

{
Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it.');
Deprecation::noticeWithNoReplacment('5.4.0');

if ($themes === null) {
$themes = SSViewer::get_themes();
}
Expand Down
15 changes: 15 additions & 0 deletions src/View/ViewableData.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,9 +426,11 @@ public function castingHelper($field)
*
* @param string $field
* @return string
* @deprecated 5.4.0 Will be removed without equivalent functionality to replace it.
*/
public function castingClass($field)
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing uses this, so no need to replace it. If we do need this in the future it can be added to the new CastingService.

{
Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it.');
Deprecation::noticeWithNoReplacment('5.4.0');

// Strip arguments
$spec = $this->castingHelper($field);
return trim(strtok($spec ?? '', '(') ?? '');
Expand All @@ -439,9 +441,11 @@ public function castingClass($field)
*
* @param string $field
* @return string 'xml'|'raw'
* @deprecated 5.4.0 Will be removed without equivalent functionality to replace it.
*/
public function escapeTypeForField($field)
Copy link
Member Author

Choose a reason for hiding this comment

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

Only one use of this in core, so no need to replace it. If we do need this in the future it can be added to the new CastingService.

{
Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it.');
Deprecation::noticeWithNoReplacment('5.4.0');

$class = $this->castingClass($field) ?: $this->config()->get('default_cast');

/** @var DBField $type */
Expand Down Expand Up @@ -488,9 +492,11 @@ public function renderWith($template, $customFields = null)
* @param string $fieldName Name of field
* @param array $arguments List of optional arguments given
* @return string
* @deprecated 5.4.0 Will be made private
*/
protected function objCacheName($fieldName, $arguments)
Copy link
Member Author

Choose a reason for hiding this comment

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

You'll see in the implementations for objCacheGet and objCacheSet that they call this directly now, so there's no need for anyone else to be calling it.

{
Deprecation::noticeWithNoReplacment('5.4.0', 'Will be made private');
return $arguments
? $fieldName . ":" . var_export($arguments, true)
: $fieldName;
Expand Down Expand Up @@ -546,6 +552,9 @@ protected function objCacheClear()
*/
public function obj($fieldName, $arguments = [], $cache = false, $cacheName = null)
{
if ($cacheName !== null) {
Deprecation::noticeWithNoReplacment('5.4.0', 'The $cacheName parameter has been deprecated and will be removed');
}
if (!$cacheName && $cache) {
$cacheName = $this->objCacheName($fieldName, $arguments);
}
Expand Down Expand Up @@ -588,9 +597,11 @@ public function obj($fieldName, $arguments = [], $cache = false, $cacheName = nu
* @param array $arguments
* @param string $identifier an optional custom cache identifier
* @return Object|DBField
* @deprecated 5.4.0 use obj() instead
*/
public function cachedCall($fieldName, $arguments = [], $identifier = null)
Copy link
Member Author

@GuySartorelli GuySartorelli Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nothing uses this, and if they did they should just call obj() directly instead.

{
Deprecation::notice('5.4.0', 'Use obj() instead');
return $this->obj($fieldName, $arguments, true, $identifier);
}

Expand All @@ -617,9 +628,11 @@ public function hasValue($field, $arguments = [], $cache = true)
* @param array $arguments
* @param bool $cache
* @return string
* @deprecated 5.4.0 Will be removed without equivalent functionality to replace it
*/
public function XML_val($field, $arguments = [], $cache = false)
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 is currently called directly from SSViewer_Scope - that will be calling methods on ViewLayerData now instead, and the methods on ViewLayerData don't rely on this method, so there's no need to keep it.

{
Deprecation::noticeWithNoReplacment('5.4.0');
$result = $this->obj($field, $arguments, $cache);
// Might contain additional formatting over ->XML(). E.g. parse shortcodes, nl2br()
return $result->forTemplate();
Expand All @@ -630,9 +643,11 @@ public function XML_val($field, $arguments = [], $cache = false)
*
* @param array $fields an array of field names
* @return array
* @deprecated 5.4.0 Will be removed without equivalent functionality to replace it
*/
public function getXMLValues($fields)
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 sure why this exists, it's not used in sink.

{
Deprecation::noticeWithNoReplacment('5.4.0');
$result = [];

foreach ($fields as $field) {
Expand Down