Is it bad practice to instantiate media model in an event handler

diana_na

I have written an event handler for the discussionsApiController_getOutput event, inorder to get media information and category name during get request.

It works as expected but could anybody advise if the following code smells or if there is a better, cleaner way to achieve my goal.

R_J

Sorry, I can not tell if there is a better way. But I can comment on your code. Vanilla is using a DI container now. You can still find old code where a model gets instantiated, but you should avoid that. You can find examples on how to do that the easiest by searching the code like that: https://github.com/vanilla/vanilla/search?q=mediamodel

As you can see, there is still $model = newMediaModel(); in the code, but you should prefer Gdn::getContainer()->get(MediaModel::class)

One other thing I see in your code is that you are using CategoryModel::categories in the loop. I would always expect the worst case: each SomeModel->someMethod() call triggers one or more database call. So instead of causing a $oneResult = select * from table where ID = X from a table with most probably only a few lines, I would prefer $result = select * from table and then use php to access that array.

This is the comment of the CategoryModel::categories method "Gets either all of the categories or a single category.". So you should better have $categories = CategoryModel::categories(); and then use $categories in your loop.

If you are writing code for Vanilla, I would suggest you also stick to Vanillas coding standard, but that's just a side note.

diana_na

@R_J I see. I will work towards correcting the things you pointed out.

As someone looking to learn to code beter, you actually provided me with the guidance I was looking for. I really appreciate your detailed explanation.

Thanks also for directing me to the coding standards.

I have an additional question if u could help. Where exactly can I hook on for expanding additional fields for discussion. I see that it's possible to expand fields like tags, category,last post and such. But I'm at a loss as to how.

$in = $this->schema(DiscussionExpandSchema::commonExpandSchema(),['DiscussionGet', 'in'])->setDescription('Get a discussion');

Following these function calls, I dont seem to see anywhere where I could hook an event handler or something

R_J

At first you need to extend the database table. After that the model will handle that column like any other.

You need to show it in the front-end by yourself, though. But if it is included in the postback data, it will be saved automatically.

Always try to find simple plugins which do something similar to what you want to do: https://github.com/vanilla/addons/blob/master/plugins/PrefixDiscussionFilter/class.prefixdiscussionfilter.plugin.php

diana_na

@R_J I apologize. I failed to explain the context of my question. Actually, I was talking about the GET discussion API V2 endpoint.

My aim isnt to expand the database structure currently. The discussion endpoint seems to allow addons to expand the input schema. But I'am not sure how. As you see the partial code for get discussion API endpoint below

The schema method does do on to fire a controller_schema event, but I assume that event is only for documentation.

The commonExpandSchema contains some of the fields i want included in the API response, but arent included by default. And I havent been able to figure how to get it done.

R_J

The QnA plugin is a complex example, therefore maybe not the best but I cannot think of any better right now. Maybe it helps to look what is done there. Use this method as an entry point: https://github.com/vanilla/vanilla/blob/master/plugins/QnA/QnAPlugin.php#L1353

R_J

The Reactions plugin is a second example

diana_na

@R_J You have been so helpful! Thank you so much!