I am working on a online newspaper/blogging application with CodeIgniter 3.1.8 and Bootstrap 4. I have decided to add themes to it. The application is not HMVC, only MVC.
I thought it was a good idea to use the Twig template engine to the theme(s). For this purpose, I use CodeIgniter Simple and Secure Twig .
The theme's templates, placed in application\views\themes\caminar
(caminar is the current theme's name) have the extension .twig
, not .php
.
The layout.twig file contains the "master layout code":
<!DOCTYPE html>
<html>
<head>
<title>{{site_title}} | {{tagline}}</title>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<link rel="stylesheet" href="{{maincss}}">
</head>
<body>
<!-- Header -->
<header id="header">
<div class="logo"><a href="/">{{site_title}}</a></div>
</header>
<section id="main">
<div class="inner">
{% if singlePost is defined %}
{{include(singlePost)}}
{% elseif pageTemplate is defined %}
{{include(pageTemplate)}}
{% elseif notFound is defined %}
{{include(notFound)}}
{% else %}
{{ include('themes/caminar/templates/posts.twig') }}
{% endif %}
</div>
</section>
<!-- Footer -->
<footer id="footer">
<div class="container">
<ul class="icons">
<li><a href="{{twitter}}" target="_blank" class="icon fa-twitter"><span class="label">Twitter</span></a></li>
<li><a href="{{facebook}}" target="_blank" class="icon fa-facebook"><span class="label">Facebook</span></a></li>
<li><a href="{{instagram}}" target="_blank" class="icon fa-instagram"><span class="label">Instagram</span></a></li>
<li><a href="mailto:{{company_email}}" class="icon fa-envelope-o"><span class="label">Email</span></a></li>
</ul>
{% if pages %}
<ul class="icons">
{% for page in pages %}
<li><a href="{{base_url}}/pages/page/{{page.id}}">{{page.title}}</a></li>
{% endfor %}
</ul>
{% endif %}
</div>
<div class="copyright">
© {{company_name}}. All rights reserved. Design by <a href="https://templated.co" target="_blank">TEMPLATED</a>
</div>
</footer>
</body>
</html>
The posts.twig file displays the posts:
<section class="wrapper style1">
{% for post in posts %}
{% if loop.first %}
<div class="image fit flush">
<a href="{{base_url}}{{post.slug}}">
<img src="{{base_url}}/assets/img/posts/{{post.post_image}}" alt="{{post.title}}" />
</a>
</div>
<header class="special">
<h2>{{post.title}}</h2>
<p>{{post.description}}</p>
</header>
<div class="content">{{post.content|raw}}</div>
{% else %}
<div class="spotlight {% if (loop.index is even) %}alt{% endif %}">
<div class="image flush">
<a href="{{base_url}}{{post.slug}}">
<img src="{{base_url}}/assets/img/posts/{{post.post_image}}" alt="{{post.title}}" />
</a>
</div>
<div class="inner">
<h3>{{post.title}}</h3>
<p>{{post.description}}</p>
<div class="{% if (loop.index is even) %}text-right{% else %}text-left{% endif %}">
<a href="{{base_url}}{{post.slug}}" class="button special small">Read More</a>
</div>
</div>
</div>
{% endif %}
{% endfor %}
</section>
<div class="pagination-container">{{ pagination|raw }}</div>
Finally, the posts controller is the main reason for my concerns regarding the quality of the code:
class Posts extends CI_Controller {
public function __construct()
{
parent::__construct();
}
private function _initPagination($path, $totalRows, $query_string_segment = 'page') {
//load and configure pagination
$this->load->library('pagination');
$config['base_url'] = base_url($path);
$config['query_string_segment'] = $query_string_segment;
$config['enable_query_strings'] =TRUE;
$config['reuse_query_string'] =TRUE;
$config['total_rows'] = $totalRows;
$config['per_page'] = 12;
if (!isset($_GET[$config['query_string_segment']]) || $_GET[$config['query_string_segment']] < 1) {
$_GET[$config['query_string_segment']] = 1;
}
$this->pagination->initialize($config);
$limit = $config['per_page'];
$offset = ($this->input->get($config['query_string_segment']) - 1) * $limit;
return ['limit' => $limit, 'offset' => $offset];
}
public function index() {
//call initialization method
$config = $this->_initPagination("/", $this->Posts_model->get_num_rows());
$data = $this->Static_model->get_static_data();
$data['base_url'] = base_url("/");
$data['pages'] = $this->Pages_model->get_pages();
$data['categories'] = $this->Categories_model->get_categories();
//use limit and offset returned by _initPaginator method
$data['posts'] = $this->Posts_model->get_posts($config['limit'], $config['offset']);
$this->twig->addGlobal('maincss', base_url('themes/caminar/assets/css/main.css'));
$this->twig->addGlobal('pagination', $this->pagination->create_links());
$this->twig->display('themes/caminar/layout', $data);
}
public function byauthor($authorid){
//load and configure pagination
$this->load->library('pagination');
$config['base_url'] = base_url('/posts/byauthor/' . $authorid);
$config['query_string_segment'] = 'page';
$config['total_rows'] = $this->Posts_model->posts_by_author_count($authorid);
$config['per_page'] = 12;
if (!isset($_GET[$config['query_string_segment']]) || $_GET[$config['query_string_segment']] < 1) {
$_GET[$config['query_string_segment']] = 1;
}
$limit = $config['per_page'];
$offset = ($this->input->get($config['query_string_segment']) - 1) * $limit;
$this->pagination->initialize($config);
$data = $this->Static_model->get_static_data();
$data['base_url'] = base_url("/");
$data['pages'] = $this->Pages_model->get_pages();
$data['categories'] = $this->Categories_model->get_categories();
$data['posts'] = $this->Posts_model->get_posts_by_author($authorid, $limit, $offset);
$data['posts_count'] = $this->Posts_model->posts_by_author_count($authorid);
$data['posts_author'] = $this->Posts_model->posts_author($authorid);
$this->twig->addGlobal('maincss', base_url('themes/caminar/assets/css/main.css'));
$this->twig->addGlobal('pagination', $this->pagination->create_links());
$this->twig->display('themes/caminar/layout', $data);
}
public function post($slug) {
$data = $this->Static_model->get_static_data();
$data['pages'] = $this->Pages_model->get_pages();
$data['categories'] = $this->Categories_model->get_categories();
$data['authors'] = $this->Usermodel->getAuthors();
$data['posts'] = $this->Posts_model->sidebar_posts($limit=5, $offset=0);
$data['post'] = $this->Posts_model->get_post($slug);
$data['author_image'] = isset($data['post']->avatar) && $data['post']->avatar !== '' ? $data['post']->avatar : 'default-avatar.png';
//CSS, JS and other resources add to twig here, because PHP and Codeigniter functions are not available from Twig templates
$this->twig->addGlobal('maincss', base_url('themes/caminar/assets/css/main.css'));
if ($data['categories']) {
foreach ($data['categories'] as &$category) {
$category->posts_count = $this->Posts_model->count_posts_in_category($category->id);
}
}
if (!empty($data['post'])) {
// Overwrite the default tagline with the post title
$data['tagline'] = $data['post']->title;
// Get post comments
$post_id = $data['post']->id;
$data['comments'] = $this->Comments_model->get_comments($post_id);
$this->twig->addGlobal('singlePost','themes/caminar/templates/singlepost.twig');
$this->twig->display('themes/caminar/layout', $data);
} else {
$data['tagline'] = "Page not found";
$this->twig->addGlobal('notFound','themes/caminar/templates/404.twig');
$this->twig->display('themes/caminar/layout', $data);
}
}
}
Concerns:
- Is the code (too) repetitive?
- Is the code over engineered?
2 Answers 2
Since your concern is chiefly falling on the controller and I don't have particular expertise with twig scripting, I'll speak on the controller. Please do not take my snippet at the end to be ready for copy-pasta -- it is not tested and it is likely that I have made mistakes and/or misunderstood the data that is being passed into these controllers. Hopefully the itemized advice will help you to realize some techniques to clean up your methods.
I prefer to declare data types on all incoming parameters and return values. Not only does this help with debugging, it also coerces values like numeric strings to ints and floats.
Rather than repeat the
$config
each time you want to declare a new element, just declare the full array in one go.Better yet, don't declare the
$config
variable at all (or any single-use variables for that matter). Any time you declare a variable then only actually use it one time, then this is a good argument for writing the value directly into where you originally had the variable. There are some exceptions to this, for instance, you want to be declarative about the value or, perhaps, directly injecting the value makes the code less readable / too wide.We shouldn't be seeing any instances of
$_GET
in your CI code. CI passes all this slash-delimited data in the url and all of the data is instantly available in your controller method's argument list. From memory, I don't think I ever use$this->input->get()
in any of my CI work.I prefer to use camelCase variable naming because PHPStorm does a good job of finding my misspellings and it is more concise than snake_case. Well, actually,
$queryStringSegment
is too verbose and only vaguely describes the value,$segment
is just as vague, but more concise. Perhaps there is a better variable name to use.You may have confused me with
/pages/page/{{page.id}}
, but I am assuming thatindex()
method is what is receiving these page load requests and the only value that you actually need is the last one{{page.id}}
. Add$pageNumber
as the lone parameter ofindex()
and pass the value along with other expected values to_initPagination()
. Ensure that$pageNumber
cannot be less than 1 -- I'll recommendmax()
.
class Posts extends CI_Controller
{
public function __construct()
{
parent::__construct();
}
private function _initPagination(string $baseUrl, int $totalRows, string $segment = 'page', ?int $pageNumber = 1): array
{
$this->load->library('pagination');
$limit = 12;
$this->pagination->initialize([
'base_url' => $baseUrl,
'query_string_segment' => $segment,
'enable_query_strings' => true,
'reuse_query_string' => true,
'total_rows' = $totalRows,
'per_page' => $limit,
]);
return [$limit, $pageNumber - 1];
}
public function index(int $pageNumber = 1): void
{
$baseUrl = base_url('/');
//call initialization method
$rangeParameters = $this->_initPagination(
$baseUrl,
$this->Posts_model->get_num_rows(),
'page',
max(1, $pageNumber),
);
$data = $this->Static_model->get_static_data()
+ [
'base_url' => $baseUrl,
'pages' => $this->Pages_model->get_pages(),
'categories' => $this->Categories_model->get_categories(),
'posts' = $this->Posts_model->get_posts(...$rangeParameters)
];
$this->twig->addGlobal('maincss', base_url('themes/caminar/assets/css/main.css'));
$this->twig->addGlobal('pagination', $this->pagination->create_links());
$this->twig->display('themes/caminar/layout', $data);
}
...
}
-
\$\begingroup\$ Please consider giving an answer to this question . Thanks! \$\endgroup\$Razvan Zamfir– Razvan Zamfir2020年11月25日 11:13:06 +00:00Commented Nov 25, 2020 at 11:13
-
\$\begingroup\$ @mickmackusa Regarding Point 6. PhpStorm (as with most IDE's) does a fine job of finding spelling mistakes regardless of which "case" style you prefer. Which means, it works just as well with snake_case as it does with camelCase etc. I understand that is your own preference but that particular reason may be misleading to some folks. I use snake_case / camelCase where applicable, due to coding/style conventions being used, under PhpStorm and it plays nice with both. \$\endgroup\$TimBrownlaw– TimBrownlaw2020年11月25日 12:48:25 +00:00Commented Nov 25, 2020 at 12:48
-
\$\begingroup\$ My point is that although camelCase smashes letters together, the IDE is smart enough to understanding/catch spelling mistakes, so I recommend the more concise variable naming convention. I have a habit of not using snake_case anywhere in my projects. This also gives an obvious separation between my entity names and native function calls. I am not misleading anyone; I said "I prefer". \$\endgroup\$mickmackusa– mickmackusa2020年11月25日 12:55:11 +00:00Commented Nov 25, 2020 at 12:55
-
1\$\begingroup\$ @Raz Just so you know, I am still interested in figuring out your bug with POST'ing from the form to the controller via ajax. I think there is a vital detail that I am not being shown. Please show me (via 3v4l.org link or similar) 1. the exact generated source code of the form that is doing the post 2. The
Network > XHR > Response tab
text from the Comments/create controller that showsexit(var_export(['stream' => $this->input->raw_input_stream, 'post' => $this->input->post(), 'slug' => $post_slug], true));
so that I can see that the controller is working. \$\endgroup\$mickmackusa– mickmackusa2020年11月26日 22:50:14 +00:00Commented Nov 26, 2020 at 22:50 -
\$\begingroup\$ @Raz This idea just popped into my head... What if you are firing two
POST
requests! What if your ajax call goes first, sends all of the POST data, then clears the$fields
, then your form's default behavior (which is not "prevent"ed) fires a secondPOST
request -- now there are no field values! and you only actually see the results of the second request. This feels very likely to me. api.jquery.com/event.preventdefault See how many requests are being fired in yourNetwork > XHR > Response tab
when you submit. \$\endgroup\$mickmackusa– mickmackusa2020年11月26日 22:56:26 +00:00Commented Nov 26, 2020 at 22:56
Well just some refactoring.
i.e. $this->twig->display('themes/caminar/layout', $data);
only needs to appear once.
When you have code that repeats inside an if
/else
, it can come outside of that block.
if (!empty($data['post'])) {
// Overwrite the default tagline with the post title
$data['tagline'] = $data['post']->title;
// Get post comments
$post_id = $data['post']->id;
$data['comments'] = $this->Comments_model->get_comments($post_id);
$this->twig->addGlobal('singlePost','themes/caminar/templates/singlepost.twig');
$this->twig->display('themes/caminar/layout', $data);
} else {
$data['tagline'] = "Page not found";
$this->twig->addGlobal('notFound','themes/caminar/templates/404.twig');
$this->twig->display('themes/caminar/layout', $data);
}
Becomes
if (!empty($data['post'])) {
// Overwrite the default tagline with the post title
$data['tagline'] = $data['post']->title;
// Get post comments
$post_id = $data['post']->id;
$data['comments'] = $this->Comments_model->get_comments($post_id);
$this->twig->addGlobal('singlePost','themes/caminar/templates/singlepost.twig');
} else {
$data['tagline'] = "Page not found";
$this->twig->addGlobal('notFound','themes/caminar/templates/404.twig');
}
$this->twig->display('themes/caminar/layout', $data);