GitHub PullRequestReviewCommentEvent at 2014-02-25 09:40:08
Repository: concrete5
Actor: mlocati
Payload: {"comment":{"url":"https://api.github.com/repos/concrete5/concrete5/pulls/comments/10029251","id":10029251,"diff_hunk":"@@ -626,12 +626,56 @@ protected function install($dir, $ptHandle, $pkgID) {\n \t\n \tpublic function getThemeID() {return $this->ptID;}\n \tpublic function getThemeName() {return $this->ptName;}\n+\t/** Returns the display name for this theme (localized and escaped accordingly to $format)\n+\t* @param string $format = 'html'\n+\t*\tEscape the result in html format (if $format is 'html').\n+\t*\tIf $format is 'text' or any other value, the display name won't be escaped.\n+\t* @return string\n+\t*/\n+\tpublic function getThemeDisplayName($format = 'html') {","path":"web/concrete/core/models/page_theme.php","position":21,"original_position":21,"commit_id":"f75ba27713e94345ac3ea9581bd9733af29211ba","original_commit_id":"f75ba27713e94345ac3ea9581bd9733af29211ba","user":{"login":"mlocati","id":928116,"avatar_url":"https://gravatar.com/avatar/f1df9349ea36e75105fac8503755434e?d=https%3A%2F%2Fidenticons.github.com%2Fc8a50610be21bf7a00a307f771e71054.png&r=x","gravatar_id":"f1df9349ea36e75105fac8503755434e","url":"https://api.github.com/users/mlocati","html_url":"https://github.com/mlocati","followers_url":"https://api.github.com/users/mlocati/followers","following_url":"https://api.github.com/users/mlocati/following{/other_user}","gists_url":"https://api.github.com/users/mlocati/gists{/gist_id}","starred_url":"https://api.github.com/users/mlocati/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mlocati/subscriptions","organizations_url":"https://api.github.com/users/mlocati/orgs","repos_url":"https://api.github.com/users/mlocati/repos","events_url":"https://api.github.com/users/mlocati/events{/privacy}","received_events_url":"https://api.github.com/users/mlocati/received_events","type":"User","site_admin":false},"body":"The same doubts can arise looking at the `t()` calls used inside the `get…Display…` methods: following the same discussion of `h()`, these `t`s too should be removed too, since we're in the model side.\r\nSo, let's suppose to remove the `t()` and `h()` from `getThemeDisplayName`: this function would be the same as `getThemeName`, so `getThemeDisplayName` should be removed.\r\nSo, in the view side we would have to write `echo h(t($theme->getThemeName()));`, and that would be fine.\r\n\r\nHowever, this is a simple case: in the other models, we have translation contexts (`TemplateFileName`, `AttributeKeyName`, `GroupName`). For instance, for group names we should write  `echo h(tc('GroupName', $group->getGroupName()));`.\r\n\r\nRemembering all the translation contexts it's quite a mess and a great source of errors. And that is the reason why @aembler asked me to introduce those `get…Display…` methods some months ago...","created_at":"2014-02-25T09:40:08Z","updated_at":"2014-02-25T09:40:08Z","html_url":"https://github.com/concrete5/concrete5/pull/1633#discussion_r10029251","pull_request_url":"https://api.github.com/repos/concrete5/concrete5/pulls/1633","_links":{"self":{"href":"https://api.github.com/repos/concrete5/concrete5/pulls/comments/10029251"},"html":{"href":"https://github.com/concrete5/concrete5/pull/1633#discussion_r10029251"},"pull_request":{"href":"https://api.github.com/repos/concrete5/concrete5/pulls/1633"}}}}