joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.76k stars 3.64k forks source link

Pagination $this->pagination->get('total') returns incorrect count in versions above 3.3.3 #4472

Closed ColinM2 closed 10 years ago

ColinM2 commented 10 years ago

Steps to reproduce the issue

This problem was found after upgrading from 3.3.3 to 3.3.6 - it is not the same as #4330 Regrettably it is not simple to reproduce as it occurs in the jDownloads series 3.2 component. You can see some reports at http://www.jdownloads.com/forum/index.php?topic=7400.0

jDownloads worked in Joomla 3.3.3 but not in Joomla! 3.3.4 onwards - I have also tried 3.3.7 dev version. It was same version of jDownloads in all cases

It manifests itself with the absence of the pagination

Using in relevant code section: $total_downloads = $this->pagination->get('total'); echo "In downloads.php circa line 107 total downloads=".$total_downloads."
";

Expected result

Using Joomla 3.3.3 - which is correct In downloads circa line 107 total downloads=25

Actual result

Using Joomla 3.3.6 - which has a problem In downloads.php circa line 107 total downloads=3

System information (as much as possible)

Joomla 3.3.3 vs Joomla 3.3.6 jDownloads 3.2.12 in all cases - no changes

Additional comments

Apologies for difficulty in testing situation but we are trying to get new updated release of jDownloads component. It is holding up release of this major component, it is very urgent

Bakual commented 10 years ago

The original issue with pagination was introduced with my PR #3945 which allowd to set limit and offset using the query builder ($query->limit and $query->offset). Due to a bug in the getListCount method, the offset was now applied to the query there which I fixed with PR #4330 (It didn't make sense to apply limit and offset in a select count anyway).

That's all I can think of which could affect your component.

ColinM2 commented 10 years ago

Thanks for comment. I 'reversed' your last edit #4330 but as I expected that made no difference. I will look at #3945 to see if that contributes! Whoops I closed by error!!

ColinM2 commented 10 years ago

Have just seen #4339 which seems very similar. It also occurred on transition between 3.3.3 and 3.3.4 This problem is not solved by #4330 which is solving another problem.

wilsonge commented 10 years ago

https://github.com/joomla/joomla-cms/pull/3725 Can you try reverting this one and see if it fixes the issue please?

ColinM2 commented 10 years ago

Thanks for input but regret reversing #3275 did not change fault. Also tested separately with both #4330 and #3275 reversed at same time - again no effect.

ColinM2 commented 10 years ago

Success! Reversing #3945 solved problem. Whether #4330 or #3275 were reversed or not had no effect. What next?

wilsonge commented 10 years ago

So #4330 should have solved the bug introduced by #3945 - so I guess we need to work out why it didn't fix things for your use case :/

Bakual commented 10 years ago

Hmm, the fix in #4330 only works if $query is an instance of JDatabaseQuery. Otherwise it would use the full query as is. However I wonder in which cases $query ends up not being a JDatabaseQuery, but still implements JDatabaseQueryLimitable.

Or is JDownloads maybe no using JModelLegacy for some reason or overrides the method?

wilsonge commented 10 years ago

JDownloads extends JModelList. getListQuery indeed returns a JDatabaseQuery object

Bakual commented 10 years ago

I tried to reproduce the issue yesterday by installing JDownloads using the webinstaller and creating some downloads and pagination worked for me on Joomla 3.3.7-dev. So I was lost. But today I saw that the webinstaller actually installs an outdated version of it (1.9.2.8 Beta).

So I can have a look later today again and see if I can figure out something.

ColinM2 commented 10 years ago

@Bakual Hi the version of jDownloads I am using is jD3.2.12 Beta see: http://www.jdownloads.com/index.php?option=com_jdownloads&Itemid=133&task=view.download&catid=43&cid=337

Bakual commented 10 years ago

Ah bummer, found why it fails. If you look at the conditional there, you see that it not only checks if the query is a JDatabaseQuery, it also checks if it's a select (obviously) and if it has a group or having part. In case of JDownloads it has a group by part and thus it uses the fallback code. But this code then has the limit applied.

I have a fix locally which seems to work. Will create a PR later.

Bakual commented 10 years ago

Please test #4485

I'm closing this issue as we have a patch. Please comment on the PR.

diskalle commented 10 years ago

Thanks for your help guys. I have now online a new jDownloads 3.2.13 beta version which worked with the original Joomla 3.3.6 files. So the users must not wait on the official next Joomla update.

About the problem: Sad to say but I had found out, that i had used a few 'group by' clauses which was not really required. So i have removed it and get now again the correct 'total results for the pagination. But for the future should be your fix very useful. Thanks.