API Deprecations for template layer #11420

GuySartorelli · 2024-10-10T03:43:51Z

Issue

Abstract template layer into its own dedicated module #11322

GuySartorelli · 2024-10-10T20:19:02Z

src/Forms/GridField/GridFieldDataColumns.php

@@ -228,16 +229,18 @@ 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)


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.

GuySartorelli · 2024-10-10T20:19:43Z

src/View/SSViewer.php

+     * @deprecated 5.4.0 Will be moved to SilverStripe\View\SSTemplateEngine.global_key
     */
    private static $global_key = '$CurrentReadingMode, $CurrentUser.ID';


Whole bunch of stuff like this that's template-specific which is getting moved over to the new engine.

GuySartorelli · 2024-10-10T20:20:11Z

src/View/SSViewer.php

+     * @deprecated 5.4.0 Will be moved to SilverStripe\View\SSTemplateEngine
     */
    protected static $topLevel = [];


We don't really have a policy about what to do with properties - I've opted to deprecate it but didn't bother mentioning it in the docs

GuySartorelli · 2024-10-10T20:21:03Z

src/View/SSViewer.php

    public function exists()
    {
+        Deprecation::notice('5.4.0', 'Will be removed without equivalent functionality to replace it');
        return $this->chosen;
    }


Literally nothing called this (except I think one place inside SSViewer itself)

GuySartorelli · 2024-10-10T20:22:12Z

src/View/SSViewer.php

        return $this->chosen;
    }

    /**
     * @param string $identifier A template name without '.ss' extension or path
     * @param string $type The template type, either "main", "Includes" or "Layout"
     * @return string Full system path to a template file
+     * @deprecated 5.4.0 Will be removed without equivalent functionality to replace it
     */
    public static function getTemplateFileByType($identifier, $type = null)


Wasn't called anywhere - and ideally people will refer to templates more loosely as candidates rather than caring about the specific files. What are you ever going to do with a full template file path in code if you're not a template rendering engine?

GuySartorelli · 2024-10-10T20:22:44Z

src/View/SSViewer.php

-     * @param ViewableData $inheritedScope The current scope of a parent template including a sub-template
+     * @param SSViewer_Scope|null $inheritedScope The current scope of a parent template including a sub-template


Just a PHPDoc so doesn't break anything to change this - and I'm changing it to what is actually being passed in here.

GuySartorelli · 2024-10-10T20:23:22Z

src/View/SSViewer.php

@@ -796,18 +865,22 @@ public function parseTemplateContent($content, $template = "")
     * 'Content' & 'Layout', and will have to contain 'main'
     *
     * @return array
+     * @deprecated 5.4.0 Will be removed without equivalent functionality to replace it
     */
    public function templates()


Literally one place called this and it was a unit test.

GuySartorelli · 2024-10-10T20:23:36Z

src/View/SSViewer.php

        return array_merge(['main' => $this->chosen], $this->subTemplates);
    }

    /**
     * @param string $type "Layout" or "main"
     * @param string $file Full system path to the template file
+     * @deprecated 5.4.0 Will be removed without equivalent functionality to replace it
     */
    public function setTemplateFile($type, $file)


Nothing called this so we clearly don't need to replace it in the new engine.

GuySartorelli · 2024-10-10T20:24:48Z

src/View/SSViewer.php

@@ -822,17 +895,29 @@ public function setTemplateFile($type, $file)
     * @param string $contentGeneratedSoFar The content of the template generated so far; it should contain
     * the DOCTYPE declaration.
     * @return string
+     * @deprecated 5.4.0 Use getBaseTag() instead
     */
    public static function get_base_tag($contentGeneratedSoFar)


Renaming this, and passing in a boolean so it's easier to use by other engines.

But for now our template parser will keep calling this (hence why it's suppressed). I've opted to not add Deprecation::withSuppressedNotice() anywhere inside the template parser, so anything that calls has a suppressed notice directly in the method being called.

GuySartorelli · 2024-10-10T20:25:25Z

src/View/SSViewer.php

+     *
+     * @param bool $isXhtml Whether the DOCTYPE is xhtml or not.
+     */
+    public static function getBaseTag(bool $isXhtml = false): string


For simple methods like this it makes sense to implement in 5.4 so people can swap to it directly if they were calling it in their code.

GuySartorelli · 2024-10-10T20:27:08Z

src/View/SSViewer_DataPresenter.php

- * 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


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.

GuySartorelli · 2024-10-10T20:28:00Z

src/View/SSViewer_FromString.php


 /**
 * 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


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.

GuySartorelli · 2024-10-10T20:29:03Z

src/View/SSViewer_Scope.php

+     * @deprecated 5.4.0 use getCurrentItem() instead.
     */
    public function getItem()
    {
+        Deprecation::notice('5.4.0', 'use getCurrentItem() instead.');


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.

GuySartorelli · 2024-10-10T20:29:26Z

src/View/SSViewer_Scope.php

+     * @deprecated 5.4.0 Will be renamed scopeToIntermediateValue()
     */
    public function obj($name, $arguments = [], $cache = false, $cacheName = null)


This is getting renamed so it's clear what it's for

GuySartorelli · 2024-10-10T20:31:44Z

src/View/ThemeResourceLoader.php

@@ -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)


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.

GuySartorelli · 2024-10-10T20:32:18Z

src/View/ViewableData.php

@@ -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)


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.

GuySartorelli · 2024-10-10T20:32:35Z

src/View/ViewableData.php

@@ -439,10 +441,13 @@ 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)


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.

GuySartorelli · 2024-10-10T20:33:13Z

src/View/ViewableData.php

@@ -488,9 +493,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)


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.

GuySartorelli · 2024-10-10T20:33:42Z

src/View/ViewableData.php

+        if ($cacheName !== null) {
+            Deprecation::notice('5.4.0', 'The $cacheName parameter has been deprecated and will be removed');
+        }


That param was never used, and it's easier to reason about the cache if the cache name is always derived from the field name and arguments.

GuySartorelli · 2024-10-10T20:33:56Z

src/View/ViewableData.php

@@ -588,9 +598,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)


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

GuySartorelli · 2024-10-10T20:36:05Z

src/View/ViewableData.php

@@ -617,9 +629,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)


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.

GuySartorelli · 2024-10-10T20:36:21Z

src/View/ViewableData.php

@@ -630,13 +644,14 @@ 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)


Not sure why this exists, it's not used in sink.

emteknetnz

I'm really not a fan of this pattern of wrapping calls to deprecated code with no replacement with Deprecation::withSuppressedNotice(), rather than just using it to wrap Deprecation::notice() in a central place

I can see why you've done it this way as it means that developers who do happen to call any of these methods will actually see the deprecation notices if they turn on deprecations. However they're pretty unlikely to be very useful as they're telling the developer to use a method that doesn't actually exist.

Wrapping calls means that there is FAR more code updates that need to be made, and we may still end up missing a call or two. Just about every other CMS 5 PR could be closed if we didn't do it this way.

Looking at our own docs - https://docs.silverstripe.org/en/5/upgrading/deprecations/#enabling-deprecation-warnings - we do explicitly say that code with no replacement won't emit a deprecation notice by default, though there is a param available to turn on those notices too.

So if it's alright, could you please simply wrap the Deprecation::notice() with Deprecation::withSupressedNotice() and stop using wrapping the calls with withSupressedNotice()

GuySartorelli · 2024-10-16T01:04:49Z

Discussed offline - we're changing the way we do these a little bit. See #11428

This is blocked until that gets merged

emteknetnz · 2024-10-21T21:41:16Z

src/View/SSViewer.php

     */
    public static function getTemplateFileByType($identifier, $type = null)
    {
+        Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it');


Suggested change

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

Deprecation::noticeWithNoReplacment('5.4.0');

emteknetnz · 2024-10-21T21:41:30Z

src/View/SSViewer.php

     */
    public function exists()
    {
+        Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it');


Suggested change

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

Deprecation::noticeWithNoReplacment('5.4.0');

emteknetnz · 2024-10-21T21:41:49Z

src/View/SSViewer.php

     */
    public static function chooseTemplate($templates)
    {
+        Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it');


Suggested change

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

Deprecation::noticeWithNoReplacment('5.4.0');

emteknetnz · 2024-10-21T21:41:57Z

src/View/SSViewer.php

     */
    public static function topLevel()
    {
+        Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it.');


Suggested change

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

Deprecation::noticeWithNoReplacment('5.4.0');

emteknetnz · 2024-10-21T21:42:31Z

src/View/SSViewer.php

     */
    public function templates()
    {
+        Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it');


Suggested change

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

Deprecation::noticeWithNoReplacment('5.4.0');

emteknetnz · 2024-10-21T21:43:03Z

src/View/ThemeResourceLoader.php

     */
    public function findTemplate($template, $themes = null)
    {
+        Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it.');


Suggested change

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

Deprecation::noticeWithNoReplacment('5.4.0');

emteknetnz · 2024-10-21T21:43:14Z

src/View/ViewableData.php

     */
    public function castingClass($field)
    {
+        Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it.');


Suggested change

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

Deprecation::noticeWithNoReplacment('5.4.0');

emteknetnz · 2024-10-21T21:43:20Z

src/View/ViewableData.php

     */
    public function escapeTypeForField($field)
    {
+        Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it.');


Suggested change

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

Deprecation::noticeWithNoReplacment('5.4.0');

emteknetnz · 2024-10-21T21:43:30Z

src/View/ViewableData.php

     */
    public function XML_val($field, $arguments = [], $cache = false)
    {
+        Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it');


Suggested change

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

Deprecation::noticeWithNoReplacment('5.4.0');

emteknetnz · 2024-10-21T21:43:36Z

src/View/ViewableData.php

     */
    public function getXMLValues($fields)
    {
+        Deprecation::noticeWithNoReplacment('5.4.0', 'Will be removed without equivalent functionality to replace it');


Suggested change

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

Deprecation::noticeWithNoReplacment('5.4.0');

GuySartorelli · 2024-10-21T21:50:29Z

Dunno if I caught all the ones you listed - but as discussed in person I've done a very basic find and replace. If I missed any... well, it's the same message anyway so it's not worth updating them.

GuySartorelli force-pushed the pulls/5/template-deprecations branch 3 times, most recently from 63b99cc to 044d44b Compare October 10, 2024 03:59

This was referenced Oct 10, 2024

ENH Suppress deprecation notices for API we can't avoid silverstripe/silverstripe-contentreview#264

Closed

ENH Avoid using deprecated API symbiote/silverstripe-advancedworkflow#555

Merged

GuySartorelli commented Oct 10, 2024

View reviewed changes

This was referenced Oct 14, 2024

ENH Avoid using deprecated API silverstripe/silverstripe-staticpublishqueue#207

Merged

ENH Suppress deprecation notices for API we can't avoid silverstripe/silverstripe-userforms#1333

Closed

emteknetnz requested changes Oct 16, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/5/template-deprecations branch 4 times, most recently from 672f9e0 to 8a7c701 Compare October 21, 2024 01:51

emteknetnz requested changes Oct 21, 2024

View reviewed changes

API Deprecations for template layer

39c5e02

GuySartorelli force-pushed the pulls/5/template-deprecations branch from 8a7c701 to 39c5e02 Compare October 21, 2024 21:50

emteknetnz approved these changes Oct 21, 2024

View reviewed changes

emteknetnz merged commit 165f72f into silverstripe:5 Oct 21, 2024
14 checks passed

emteknetnz deleted the pulls/5/template-deprecations branch October 21, 2024 23:52

		* @param ViewableData $inheritedScope The current scope of a parent template including a sub-template
		* @param SSViewer_Scope\|null $inheritedScope The current scope of a parent template including a sub-template

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

API Deprecations for template layer #11420

API Deprecations for template layer #11420

Conversation

GuySartorelli commented Oct 10, 2024

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment • edited Loading

Choose a reason for hiding this comment

GuySartorelli commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Oct 21, 2024

GuySartorelli Oct 10, 2024 •

edited

Loading

GuySartorelli Oct 10, 2024 •

edited

Loading

emteknetnz left a comment •

edited

Loading

GuySartorelli commented Oct 16, 2024 •

edited

Loading