It’s good to see your class because it makes it clear where the problems are and as I suspected there seem to be too many responsibilities in it. You call this class NewsList, which indicates its purpose is to get a list of news or perhaps manage the list in some other ways but then you say:
so then it turns out the object does not only fetch a news list but also other things that constitute “full information for a Smarty template”. If that is the case then you should name your class NewsTemplateData or something like this.
But let’s get back to NewsList and what the main and only method returns:
$fields['page_title'] = $this->page_title;
$fields['see_all_label'] = $this->see_all_label;
$fields['see_all_href'] = $this->see_all_href;
$fields['data'] = $this->data;
$fields['pager'] = $this->pager->retrieve();
$fields['photo'] = $this->photo;
To me data
is the only key that actually is what I would call a news list - and possibly photo
if we consider photos as part of the news items. Page title, label and href are data unrelated to the news list and the NewsList class shouldn’t be responsible for that. The NewsList class shouldn’t know or care that you want to display the news list in a Smarty template and neither should it care about the template’s title, label and other information. What if later on you want to get the news list for RSS feeds or consolidate the list into a spreadsheet file or even display it on another page with a different title and no labels, hrefs, etc.? Sure, you could use the same class but then it’s messy because the class does a bunch of things which are unnecessary and to avoid redundant code execution you would need to create a separate class. Reusability suffers.
The same thing is with pager
- even though pagination is related somehow to the list I would consider it a separate responsibility because depending on context you may or may not want any pagination, or you may want pagination of different kind, etc. That’s why I would move pagination to a separate class.
Then the usage for your web page would be roughly like this:
$newsList = new NewsList($db, $site);
$pager = new NewsListPager($_GET['page'], $newsList->countAllItems());
$newsItems = $newsList->retrieve($pager->startingItemIndex(), $pager->limitOnPage());
And then you would pass $pager
and $newsItems
to your template. Your template then would foreach
over newsItems
to display the news list and fetch all necessary pagination info from the pager
object to display page navigation. Of course, the pager should have all necessary methods to provide information like getPageLinks()
, getTotalPages()
, etc.
You can now see how the NewsList class requires much fewer dependencies because it is dealing with one responsibility only. To your retrieve()
method you provide information what range of items to display and the method would create appropriate LIMIT clause for the database query. In a more sophisticated case you could also pass search criteria, etc. in the same way.
There would be no need for a separate photo
object because to me photos are logically part of news items, if each news item has one photo then simply let the retrieve
method add a photo
element to the array that contains the photo url, dimensions, etc. A photo is part of an item’s data the same way as headline, date_start, teaser, etc.
This is just a general idea how to separate your class into two in order to avoid mixing concerns. As always there are many ways to skin a cat 
There are two more issues in your code I want to mention:
Problem 1: You should avoid using new
in your classes:
$this->query = new Query;
The new
keyword is like a global variable and it results in hidden dependencies that are not visible nor obvious from the outside. Technically, you should also pass the query object in the constructor. However, to me this looks like a reverse case of the previous problem and I would not separate the query object from your database object so much because this means you would need to pass two objects, db
and query
, to all classes that query the database. I’m sure you don’t ever need to build an SQL query without sending it to a database so the query builder does not need to be passed separately. Instead, you can add a factory to the db
class to create the query object on demand. And usage in your class would be like this:
$query = $this->db->getQueryBuilder();
$query->table("news");
$query->select("n.id, n.headline, n.teaser, n.pub_start, n.pub_end, n.url");
$query->from("news n");
$query->where("n.site_id = ".$this->site->id);
$query = $query->getQuery();
$records = $this->db->select($query);
And now you just need to require db
as a dependency of your class. Another benefit is that you can easily create as many query objects as you like whereas in your code you were limited to only one query (unless you implemented some other workarounds).
Problem 2:
// If article has a photo, store info about it in Photo object.
// ->setPhotoID() will initiate a new array within object that stores photo info
if (isset($rec['photo_id'])) {
$this->photo->setPhotoID($rec['photo_id']);
$this->photo->setFolder($rec['folder']);
$this->photo->setFilename($rec['filename']);
$this->photo->setW($rec['dim_x']);
$this->photo->setH($rec['dim_y']);
$this->photo->setThumbFilename($rec['thumbname']);
$this->photo->setThumbW($rec['thumb_x']);
$this->photo->setThumbH($rec['thumb_y']);
$this->photo->setCaption($rec['caption']);
$this->photo->setPhotog($rec['photog']);
$this->photo->setContentID($rec['id']);
}
This kind of code is very misleading and if I didn’t read the comment “setPhotoID() will initiate a new array within object that stores photo info” I would have thought the code was doing something completely different. It’s a hidden surprise that setPhotoID() creates a new array. It’s also a hidden surprise that object Photo
holds more than one photo! If it holds more than one photo then name it Photos or PhotoList, etc. And when you add a photo to a PhotoList object then first create the photo array (or object) and then add it with a method like $photoList->addPhoto($photo)
- and then all becomes clear without the need for commenting code! Obviously, this is not relevant here any more because we got rid of the strange Photo
object (at least in this form) but something to keep in mind in general.