[RFC] Use the PageModel instead of direct SQL queries in button callbacks #1090
Conversation
|
There is no reason and this is an excellent PR, thank you very much! |
src/Resources/contao/dca/tl_page.php
Outdated
| $objSubpages = $this->Database->prepare("SELECT * FROM tl_page WHERE pid=?") | ||
| ->limit(1) | ||
| ->execute($row['id']); | ||
| $objSubpages = \PageModel::findByPid($row['id'], array('limit'=>1)); |
There was a problem hiding this comment.
You could actually use countBy here, because we only need to know if there are subpages.
There was a problem hiding this comment.
But then it's not read from the model registry and an extra sql query is executed (see Model.php).
There was a problem hiding this comment.
Well it does not read from the registry this way either... findByPid is not cacheable, and nothing is cacheable if you use options.
There was a problem hiding this comment.
You're totally right. It's only read from registry if a model is requested by it's id.
But one question is left: Is there a (measurable) difference in performance, when setting a limit of one or query all child records and count them?
I don't think so. The amount of data from the sql server is negligible unless someone have a thousand sub pages. So I'm fine with using countBy. Looks more logic.
There was a problem hiding this comment.
Changed to the countByPid method in b89e330
agoat
force-pushed
the
fix/reduceSqlQueriesOnTableCallbacks
branch
from
bc5dd53
to
b62d2fb
Compare
| $objPage = $this->Database->prepare("SELECT * FROM tl_page WHERE id=?") | ||
| ->limit(1) | ||
| ->execute($row['pid']); | ||
| $objPage = \PageModel::findById($row['pid']); |
There was a problem hiding this comment.
The namespace prefix is not necessary here.
| $objPage = $this->Database->prepare("SELECT * FROM " . $table . " WHERE id=?") | ||
| ->limit(1) | ||
| ->execute(Input::get('id')); | ||
| $objPage = \PageModel::findById(Input::get('id')); |
There was a problem hiding this comment.
This is wrong. You have to use the model that corresponds to $table.
| $objPage = $this->Database->prepare("SELECT * FROM " . $table . " WHERE id=?") | ||
| ->limit(1) | ||
| ->execute($row['pid']); | ||
| $objPage = \PageModel::findById($row['pid']); |
| $objSubpages = $this->Database->prepare("SELECT * FROM tl_page WHERE pid=?") | ||
| ->limit(1) | ||
| ->execute($row['id']); | ||
| $subPages = \PageModel::countByPid($row['id']); |
There was a problem hiding this comment.
This has no effect. If you want to reuse the existing model, you have to use findByPid() here.
|
You're welcome. |
This PR handles the optimisation of button callbacks mentioned in #1089.
Is there a reason why all the button callbacks don't use Models??
Because if we will do, the amount of SQL Queries will drop from 415 to around 210 in the article view of the official contao demo page.
Currently every rendered article row will call the buttons callbacks which itself will make the same SQL Query to the tl_page table for every page (around 6 queries per article).
When the
\PageModelis used, the Result is cached.