Keeping the Data Layer Honest
Keep queries out of controllers, logic out of resource models, and turn the N+1 in a collection loop into one query.
Three rules, in the order they usually get broken.
No queries in a controller
A controller reads the request and returns a result. The lookup belongs behind a repository or a small service class injected into it. A collection built inside a controller cannot be reused, cannot be unit tested without a database, and gets copied into the next controller that needs the same rows.
No business logic in a resource model
A resource model maps a row to a model and back. Price decisions, status transitions and validation belong in a model, in the repository's save path, or in a service of their own. Logic hidden in _beforeSave() runs on every path that touches the entity and is invisible to anyone reading the caller.
The N+1
A loop over a collection that calls a repository inside the loop issues one query per row. Five hundred rows means five hundred and one queries, and the page is slow only on real data. The fix is always the same: collect the ids, make one call with an in filter, index the result by id, then loop over the rows you already have.
The same shape hides in getSize() inside a loop, in $model->save() once per row, and in loading whole models to read one column — addFieldToSelect() exists for that.
These are PHP-only changes. bin/magento cache:flush is enough in developer mode; after a constructor changes, production mode needs bin/magento setup:di:compile.
Example
<?php
// app/code/Vendor/Faq/Model/QuestionAuthors.php
declare(strict_types=1);
namespace Vendor\Faq\Model;
use Magento\Customer\Api\CustomerRepositoryInterface;
use Magento\Framework\Api\SearchCriteriaBuilder;
use Vendor\Faq\Api\Data\QuestionInterface;
use Vendor\Faq\Model\ResourceModel\Question\CollectionFactory;
class QuestionAuthors
{
public function __construct(
private readonly CollectionFactory $collectionFactory,
private readonly CustomerRepositoryInterface $customerRepository,
private readonly SearchCriteriaBuilder $searchCriteriaBuilder
) {
}
/**
* @return array<int, string> question id => author email
*/
public function getAuthorEmails(): array
{
$collection = $this->collectionFactory->create();
$collection->addFieldToSelect(['question_id', 'customer_id']);
$collection->addFieldToFilter('is_active', 1);
$questions = $collection->getItems(); // query 1
// Shorthand and longhand for the same function:
$ids = array_map(fn (QuestionInterface $q) => (int)$q->getCustomerId(), $questions);
// $ids = array_map(function (QuestionInterface $q) {
// return (int)$q->getCustomerId();
// }, $questions);
$criteria = $this->searchCriteriaBuilder
->addFilter('entity_id', array_values(array_unique($ids)), 'in')
->create();
$emails = [];
foreach ($this->customerRepository->getList($criteria)->getItems() as $customer) {
$emails[(int)$customer->getId()] = (string)$customer->getEmail();
} // query 2
$result = [];
foreach ($questions as $question) {
$id = (int)$question->getCustomerId();
$result[(int)$question->getId()] = $emails[$id] ?? '';
}
return $result;
}
}
// Output: two queries for any number of questions.When to use it
- Fix a storefront page that is fast locally and slow on production data.
- Move a query out of a controller so a cron job can reuse it.
- Review a module for queries hidden in loops before it ships.
More examples
The N+1 you are looking for
Every repository or model call inside a loop over rows is this bug. It passes code review because each line is correct on its own, and it passes testing because the dev database has twelve rows. Look for getById, load(), a magic getter that lazy-loads a related entity, or getSize() between a foreach and its closing brace.
<?php
// The shape to recognise in review. Do not ship this.
$collection = $this->collectionFactory->create();
$collection->addFieldToFilter('is_active', 1);
$emails = [];
foreach ($collection as $question) {
// One SELECT per question, every time round the loop.
$customer = $this->customerRepository->getById((int)$question->getCustomerId());
$emails[(int)$question->getId()] = $customer->getEmail();
}
// 1 query for the collection + 1 per row.
// 500 active questions: 501 queries, all of them avoidable.A controller with no query in it
The controller's whole job is routing a request to a service and choosing a result type. Because the query lives in QuestionAuthors, the same lookup serves a console command, a cron job and a REST endpoint without being written again, and it can be tested by constructing the service with a fake repository.
<?php
// app/code/Vendor/Faq/Controller/Question/Authors.php
declare(strict_types=1);
namespace Vendor\Faq\Controller\Question;
use Magento\Framework\App\Action\HttpGetActionInterface;
use Magento\Framework\Controller\Result\JsonFactory;
use Vendor\Faq\Model\QuestionAuthors;
class Authors implements HttpGetActionInterface
{
public function __construct(
private readonly JsonFactory $resultJsonFactory,
private readonly QuestionAuthors $questionAuthors
) {
}
public function execute()
{
$result = $this->resultJsonFactory->create();
return $result->setData($this->questionAuthors->getAuthorEmails());
}
}
// GET /faq/question/authors returns 200 with a JSON object of
// question id to email. The controller holds no collection, no
// filter and no table name.A resource model with nothing in it but mapping
Keeping the resource model to its _construct() is a deliberate choice, not laziness. Anything placed in _beforeSave or _afterLoad fires for every caller including imports, data patches and the API, in an order nobody reading the calling code can see. Validation that must always run belongs in the repository's save path, where it is visible and testable.
<?php
// app/code/Vendor/Faq/Model/ResourceModel/Question.php
declare(strict_types=1);
namespace Vendor\Faq\Model\ResourceModel;
use Magento\Framework\Model\ResourceModel\Db\AbstractDb;
class Question extends AbstractDb
{
protected function _construct()
{
$this->_init('vendor_faq_question', 'question_id');
}
// Not here:
// _beforeSave() that rewrites the title
// _afterLoad() that sends a notification
// a sortQuestionsForDisplay() helper
//
// Those belong in the model, in QuestionRepository::save(), or in a
// service class the repository calls.
}
Discussion