Hi,

Just wanting to get your opinions on what is a better MVC partitioning of the MetaTags generation code...

At present, I've extended the SiteTree::MetaTags(...) method to add some custom meta tags, so it looks like:

Current Code
Page.php

class Page extends SiteTree {
	public function MetaTags($includeTitle = true) {
		$tags = "";
		if($includeTitle === true || $includeTitle == 'true') {
			$tags .= "<title>" . Convert::raw2xml(($this->MetaTitle)
				? $this->MetaTitle
				: $this->Title) . "</title>\n";
		}

		$tags .= "<meta name=\"generator\" content=\"SilverStripe - http://silverstripe.org\" />\n";

		$charset = ContentNegotiator::get_encoding();
		$tags .= "<meta http-equiv=\"Content-type\" content=\"text/html; charset=$charset\" />\n";
		if($this->MetaKeywords) {
			$tags .= "<meta name=\"keywords\" content=\"" . Convert::raw2att($this->MetaKeywords) . "\" />\n";
		}
		if($this->MetaDescription) {
			$tags .= "<meta name=\"description\" content=\"" . Convert::raw2att($this->MetaDescription) . "\" />\n";
		}

                <snip>

		if($this->ExtraMeta) { 
			$tags .= $this->ExtraMeta . "\n";
		} 

		$this->extend('MetaTags', $tags);

		return $tags;
	}

But I'm wondering since the method is doing a bit of the rendering (ie the View) whether the code should be shifted across to a template? Eg.

More MVC compliant?
Page.php

class Page extends SiteTree {
	public function MetaTags($includeTitle = true) {
		$titleToRender = "";
		if($includeTitle === true || $includeTitle == 'true') {
			$titleToRender = ($this->MetaTitle)
				? $this->MetaTitle
				: $this->Title);
		}

		$charset = ContentNegotiator::get_encoding();

		$this->extend('MetaTags', $tags);

		$customFields = array();
		$customFields['titleToRender'] = $titleToRender;
 		$customFields['charset'] = $charset;

		return $this->renderWith('MetaTagsTemplate', $customFields);
	}
}

MetaTagsTemplate.ss

    <meta charset="$charset" />
    <meta name="generator" content="SilverStripe - http://silverstripe.org" />
    <meta name="robots" content="all" />
    <%if MetaDescription %><meta name="description" content="$MetaDescription" /><% end_if %>
    <%if MetaKeywords %><meta name="keywords" content="$MetaKeywords" /><% end_if %>    
    <%if ExtraMeta %>$ExtraMeta<% end_if %>     

What do you think? I'm inclined to go for the template approach. Is there anything wrong with this approach? Anything I've overlooked?

Thanks for your input.

VWD.

I like the idea to make it a template as it would allow people to easily customize it without manually butchering the function and keeps the html out of the PHP (which is always a good thing!)

It would be great if you could submit your work as a patch to the github (to sapphire/master) so the community can review it. I see one issue with your code and that is your extension call passes an undefined variable and doesn't use that in the render with the it renders the extension hook useless. I'm not use you even need an extension hook as you could extend it via theming but then again it'll get tricky if you have 3 modules all trying to update the meta tags if you use a sub theme.

From a style point of view, I would also call it MetaTags.ss rather than MetaTagsTemplate.ss as the template is implied. I would also do the MenuTitle vs Title check in the template.