Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Put back missing ORDER BY so that ssi_recentTopics is ordered as per SMF2.0. #5114

Merged
merged 3 commits into from Nov 6, 2018

Conversation

tinoest
Copy link
Contributor

@tinoest tinoest commented Nov 4, 2018

Signed-off-by: tinoest tinoest@gmail.com

@albertlast
Copy link
Collaborator

In my eyes not needed,
Do the sort Manuel after this by using php.

@tinoest
Copy link
Contributor Author

tinoest commented Nov 4, 2018

This breaks compatibility with the old way of doing things. It’s also more efficient to do it in the SQL than php. Not ignoring the fact you don’t have the key to sort on anyway.

@albertlast
Copy link
Collaborator

Sql didn't allow to sort col,
Which you didn't have in select.
So this query doesn't meet the requirement.

@tinoest
Copy link
Contributor Author

tinoest commented Nov 4, 2018

Sql didn't allow to sort col,
Which you didn't have in select.
So this query doesn't meet the requirement.

Added it to the SELECT so that it works as expected.

@albertlast
Copy link
Collaborator

Sry when I confused you,
You're free to add the sort,
When you add this col to the select,
Like now,
You could add sort.
Without sort it's "random" if the value sorted or not.

@live627
Copy link
Contributor

live627 commented Nov 5, 2018

Simple test script

<?php

require_once('./SSI.php');

ssi_recentTopics();
displayDebug();

This fixes the ordering so that latest posts are always first. I forgot to do that way back when I split the queries (do a git blame). Now a filesort is performed.

@Sesquipedalian Sesquipedalian merged commit 8cd8cca into SimpleMachines:release-2.1 Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants