Skip to main content

This site requires you to update your browser. Your browsing experience maybe affected by not having the most up to date version.

General Questions

General questions about getting started with SilverStripe that don't fit in any of the categories above.

Moderators: martimiz, Sean, biapar, Willr, Ingo, swaiba, simon_w

Which is better MVC approach for MetaTags?


Reply

2 Posts   961 Views

Avatar
vwd

4 November 2011 at 11:46am (Last edited: 4 November 2011 11:50am), Community Member, 159 Posts

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.

Avatar
Willr

4 November 2011 at 6:19pm Forum Moderator, 5511 Posts

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.