search

mybbgroup / Thank-you-like-system

Thank you/Like system with MyAlerts support
https://mybb.group/Thread-Thank-you-like-system
GNU General Public License v3.0
11 stars 12 forks source link

Modify code to follow MyBB v1.8.23 changes - new hook in forumdisplay #225

Closed Eldenroot closed 4 years ago

Eldenroot commented 5 years ago

There will be a new hook in the upcoming release of MyBB - v1.8.22.

It will be for forumdisplay.php so we can replace this "hack" and use a new hook.

https://github.com/mybbgroup/Thank-you-like-system/commit/a2e81f30093420185c13974ffd241be8bea1f2ee#diff-61561fd3f118cc16cdc5c0bbb79b38c7R1808

MyBB related PR: https://github.com/mybb/mybb/pull/3805

Eldenroot commented 4 years ago

@SvePu - what do you think, is this OK? If yes, I will create a PR to MyBB repo

https://github.com/veryard/mybb/commit/149d4329cdc5631ee319a12a8efb09692aad788e

Eldenroot commented 4 years ago

@SvePu - it was merged into MyBB 1.8.23 - with this new hook we can replace "old hacky way"

Eldenroot commented 4 years ago

@lairdshaw - buddy, cal I ask you to look at this? Now there is a much easier way (nicer) to use newly added hook for forumdisplay_threadlist.

Thank you very much!

lairdshaw commented 4 years ago

Hey @Eldenroot - I didn't write this code and haven't analysed it carefully, but my initial thoughts are: the hook currently being used is the appropriate one. If we were to use the new hook, all of the 'forumdisplay_thread' templates, which are the ones we need to modify, would have already been rendered (into the $threads variable), meaning we would have to re-compute them (inefficient and pointless), along with the 'forumdisplay_threadlist' template, which would already have been rendered into the $threadslist variable (and again, it would be inefficient and pointless to re-compute this variable).

That said, the existing code looks like it could be optimised in that looks as though the $tyl_forumdisplay_cached variable is only ever set to false, such that it does not perform any caching role at all, which means that a redundant database query is performed for each thread in the listing, when it looks to me (though without careful examination) as though only one is required if caching were to work "as advertised".

Eldenroot commented 4 years ago

OK, can I kindly ask you to look at this?

Btw - I installed your plugin for URLs yesterday and now I am writting down my ideas on a paper... give me few more days, need to consider all and cover all possible features/improvements... still WIP in my head.

lairdshaw commented 4 years ago

OK, can I kindly ask you to look at this?

OK. I'll see if I can eliminate redundant DB queries, assuming I'm correct that they exist.

Btw - I installed your plugin for URLs yesterday and now I am writting down my ideas on a paper... give me few more days, need to consider all and cover all possible features/improvements... still WIP in my head.

Much appreciated - thank you!

lairdshaw commented 4 years ago

The commit I've just added seems to work as expected. See what you think.

Eldenroot commented 4 years ago

Tested briefly, works fine, thank you!