Using SQL queries in Theme template

[Yojin_Spajin]

Hi,

I'm trying to use an SQL Query to display some precise subject in the profil page template. I'm using SMF 1.1.x.

In my Profile.Template.php I've put this :

Code Select Expand

function template_forumProfile()
{
global $context, $settings, $options, $scripturl, $modSettings, $txt, $smcFunc;
echo $smcFunc;
echo "Query ?";
$request = $smcFunc['db_query']('', '
SELECT smf_messages.ID_TOPIC, smf_messages.subject
FROM smf_messages, smf_topics
WHERE smf_topics.ID_FIRST_MSG = smf_messages.ID_MSG
AND smf_topics.ID_BOARD != 37
AND smf_topics.ID_BOARD != 36
AND smf_topics.ID_BOARD != 35
AND smf_topics.ID_BOARD != 34
AND smf_topics.ID_BOARD != 38
AND smf_topics.ID_BOARD != 2
AND smf_topics.ID_BOARD != 3
AND smf_topics.ID_BOARD != 47
AND smf_topics.ID_BOARD != 48
AND smf_topics.ID_BOARD != 4
AND smf_messages.ID_MEMBER = 3822
GROUP BY smf_messages.subject
',
array());
echo "It works";
        //... (and more code)
}

> "Query ?" is displayed, but "It works" (and the code after) is not. The request makes it crash, I suppose.

This request works well in phpmyadmin.

I probably make some mistake, or act in a wrong way...

Can you help ? Thank !

[vbgamer45]

$smcFunc['db_query'] is for SMF 2.0.x

[Yojin_Spajin]

Well this is strange, I've found it in my Sources/subscriptions.php. Maybe this file should not be there (a trace of a failed installation ? I'm not sure what tried to do former admins, meh). I'll try with 1.1 func, so.

[Arantor]

subscriptions.php is not a file even part of a stock 1.1 installation...

For 1.1, you want db_query() instead, but it *still* should never be part of a template... ever.

[Yojin_Spajin]

Works better like that !

Code Select Expand

function template_forumProfile()
{
global $context, $settings, $options, $scripturl, $modSettings, $txt;
$request = db_query('
SELECT smf_messages.ID_TOPIC, smf_messages.subject
FROM smf_messages, smf_topics
WHERE smf_topics.ID_FIRST_MSG = smf_messages.ID_MSG
AND smf_topics.ID_BOARD not in(38,37,36,35,34,2,3,4,47,48)
AND smf_messages.ID_MEMBER = 3822
GROUP BY smf_messages.subject
',__FILE__, __LINE__);
$array = array();
while ($data = db_fetch_assoc($request))
echo $data;
$array[] = $data;
db_free_result($request);
}


But still crashing on the first db_fetch_assoc...

Arantor > Why it should not ?

[vbgamer45]

If it's the users own site doesn't matter. the templates are php code anyways not a real template system.

I would clean this part of the query up though

Code Select Expand


AND smf_topics.ID_BOARD != 37
AND smf_topics.ID_BOARD != 36
AND smf_topics.ID_BOARD != 35
AND smf_topics.ID_BOARD != 34
AND smf_topics.ID_BOARD != 38
AND smf_topics.ID_BOARD != 2
AND smf_topics.ID_BOARD != 3
AND smf_topics.ID_BOARD != 47
AND smf_topics.ID_BOARD != 48
AND smf_topics.ID_BOARD != 4

To

Code Select Expand


AND smf_topics.ID_BOARD not in(38,37,36,35,34,2,3,4,47,48)


[Arantor]

Because it's absolutely terrible practice.

There is a fundamental design principle to separate logic and presentation. It is a good habit to get into and a terrible one to break out of.

Just because something isn't a 'real template system' does not mean that is an excuse to ignore the design principles of SMF itself, or indeed to ignore good developmental practice. These things are in the coding guidelines for a reason (and I would note if your mods were ever to be reviewed again, they would never pass the review process for that, and many other reasons)

[Yojin_Spajin]

VBGamer45 > Haha, you are welcome, thank.

Arantor > Maybe there is an other cleaner way to display it only for a specific theme, but well, we are using each theme like an isolated sandbox, even in the offered features, so this much more convenient for us.

Then, It still doesen't work, mh, anyone ?

[Arantor]

There's no db_ function for fetch_assoc in 1.1, you would use db_query to perform the query and mysql_fetch_assoc to fetch the data. This is deprecated in PHP itself in 5.5 and may be removed in future versions without notice.

Even if you use a single theme as a sandbox, it's still better practice to isolate logic from presentation.

[vbgamer45]

Arantor sure there maybe better practices but 90% people don't need it. For SMF core sure but for their own sites it doesn't matter just do what gets the job done. And honestly the mod approval process should be scrapped it is way too tough and scares newbies away and why we don't have many mod authors around any more. Very few people ever look at the code they just want something that works. 

Also just did a quick scan on *.template.php on my mods on the modsite only one has two queries in the template which i will fill fix.

[Yojin_Spajin]

Hi again !

I think we are almost done ! Works great with the request I've shown to you. But when I try with this request :

Code Select Expand

SELECT topics_rp.subject
FROM smf_messages, (
SELECT smf_messages.ID_TOPIC, smf_messages.subject
FROM smf_messages, smf_topics
WHERE smf_topics.ID_FIRST_MSG = smf_messages.ID_MSG
AND smf_topics.ID_BOARD not in(38,37,36,35,34,2,3,4,47,48)
) topics_rp
WHERE smf_messages.ID_TOPIC = topics_rp.ID_TOPIC
AND smf_messages.ID_MEMBER = 1557
GROUP BY topics_rp.subject

I've an "Hacking attempt..." error that appears... !

[vbgamer45]

Your using subqueries call this before your query

Code Select Expand


global $modSettings;
$modSettings['disableQueryCheck'] = true;


[Arantor]

@Yojin_Spajin: SMF does not allow for subselects.

That said, it seems a *very* inefficient way of doing it to me, and you may want nearer to:

Code Select Expand


SELECT t.ID_TOPIC, m.subject
FROM smf_topics AS t
INNER JOIN smf_messages AS m ON (t.ID_FIRST_MSG = m.ID_MSG)
WHERE t.ID_BOARD NOT IN (38, 37, 36, 35, 34, 2, 3, 4, 47, 48)
AND t.ID_MEMBER_STARTED = 1557


See, as I understand your query, you're getting the possible list of topics started by a given member in any of those boards (based on the conjoining of the tables) and doing it in a very inefficient way - since grouping on subject is about the most expensive way you can do it, especially as it has to group by subject after manually filtering the messages table, rather than filtering the much smaller topics table before getting the message data.

This way we filter the topics to those started by that member (since it will be the same as the ownership of the first message) in those boards, and then fetch the subject, rather than fetching everything and expecting MySQL to filter it back down the other way.

And you don't have to disable the rest of SMF's security (which in 1.1 is actually much more useful than it is in 2.0 since 2.0 does have query parameterisation to prevent SQL injections, at least it does when mod authors actually bother to use it properly)



@vbgamer:

Except you fail to understand why I push for correctness.

Getting people into good habits makes everything easier in the long run - and it means if they already have the concept of separating behaviours in their mind, they will have an idea where to look for code in the future.

It's like indentation. It seems like a pain when you're first getting the hang of it, but once you have, you begin to understand what benefits it gives you and it feels natural and automatic to do it sanely.

As for 'don't need it', this is the reason TDWTF is even a thing. This is why there are so many pieces of broken software out there. So many bugs in so many pieces of software because 'it gets the job done' as opposed to actually doing it right.

Like your gallery for example. The comment system 'just works' unless you want to use certain types of bbcode because you don't actually sanitise it properly. I did consider actually filing a security report because between the queries in templates, the improper use of $smcFunc and not using preparsecode, I'm not sure there actually isn't a security hole waiting to happen.

I was seeing many queries on a page that could have been done in 2, and it was in the template. You do not need to run loadMemberData every iteration of the comment list. Go through the comments when you load them (in the source), grab the member id as you go along, then call loadMemberData once with all the ids at once to reduce the total number of queries to 2 rather than 2 for every unique poster that made a comment on an item. This is a *prime* example of what I'm getting at. By deferring the logic to the template it actively discourages you from making better use of the tools available to you. Certainly on a very busy site this could contribute to a DOS attack.

I realise I'm sounding like a hard-ass but at the same time I've worked amongst a good many developers and a good many development styles and I know that adhering to coding standards tends to encourage a culture of reducing bugs being created before having to be dealt with.

[Yojin_Spajin]

You are awesome, thank you a lot.

My query finds all topics that do not belong to certain category in which a specific member has posted at least one message, in fact (not only those which he is the author of the first message, as my first request, and yours, do).

[Arantor]

Ah, see, this wasn't exactly clear, especially since the join actually implies it wouldn't do that (but I'm doing it off the top of my head, not looking at an actual database, and it is like 3am... so yeah, I could well be wrong)

[Yojin_Spajin]

Thank you anyway.