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

Fix articles limit for category view #32626

Closed

Conversation

joomdonation
Copy link
Contributor

Pull Request for Issue #18529 and #32612 .

Summary of Changes

This PR proposes a solution to fix the two issues #18529 and #32612. Basically, it now will only load articles if limit > 0 (users want to display articles using the category layout)

See #18529 and #32612 to understand the actual issue. However, this PR has a backward incompatible change, so I'm unsure if it should be accepted (especially for staging).

Testing Instructions

  1. Create two menu items, one links to Category Blog Layout menu item type and one to Category List menu item type.
  2. Apply patch, confirm that two menu items still display articles same as before.

Backward Incompatible Changes

Please note that there is a backward incompatible changes here then the site has no menu item to link to one of the three menu item types (basically, no suitable menu item could be found to link to a category)

  • List All Categories
  • Category Blog Layout
  • Category List

For example, when you unpublish all menu items (keep home menu but links to a different component than com_content) and access to a category display in Articles - Categories. Before this PR, it will display all articles from that category, after this PR, only number of articles will be displayed (controlled by Default List Limit parameter in Global Configuration)

@AndySDH
Copy link
Contributor

AndySDH commented Mar 10, 2021

Thanks for the effort!

Unfortunately, I tested this unsuccessfully. This results in this error when viewing a category with 0 articles displayed:

0 - Call to a member function getPagination() on null

@joomdonation
Copy link
Contributor Author

@AndySDH Will look at it later.

@trananhmanh89
Copy link
Contributor

trananhmanh89 commented Mar 11, 2021

@joomdonation should we return empty array at category model when $limit == 0 ?

or return empty array on ListModel.php, because this issue can affect other core components.

@joomdonation
Copy link
Contributor Author

Should we return empty array at category model when $limit == 0 ?

I have to look at it again but I think it is working like that for category model at the moment

return empty array on ListModel.php, because this issue can affect other core components

It is a backward-incompatible change, so we are not allowed to do that. Further more, in Joomla!, look like we want all records returned when no limit is set.

@trananhmanh89
Copy link
Contributor

I think we only need change line 251 of components\com_content\models\category.php

from if ($limit >= 0)

to if ($limit > 0)

@joomdonation
Copy link
Contributor Author

I think we only need change line 251 of components\com_content\models\category.php

from if ($limit >= 0)

to if ($limit > 0)

I don't think that's enough. It will cause no articles being displayed when access to a category from Articles - Categories module (with no menu items linked to categories or category view). Also, if we just change that, there are still unnecessary code executed ($model->setState commands) when we do not need to query database to get articles.

I will try to review code carefully later.

@trananhmanh89
Copy link
Contributor

Correct me if i'm wrong. Your code is trying to resolve unlimited items on two cases:

1. Menu com_content, view=category and all settings are set to 0 item

  • Your solution: don't call model->getItems() when limit <= 0
  • My point: It's ok. And i found that the pagination still load and make query to get total. Is that ok?

2. You access com_content, view=category without any menu

  • We can use this url to replicate : index.php?option=com_content&view=category&id={category_id}
  • Your solution: set limit to value of global config list_limit
  • My point: why don't we use global config of com_content in this case? May be this check no longer needed?

// Set limit for query. If list, use parameter. If blog, add blog parameters for limit.
if (($app->input->get('layout') === 'blog') || $params->get('layout_type') === 'blog')

@AndySDH
Copy link
Contributor

AndySDH commented Mar 11, 2021

  1. Exactly, Category Blog has default settings, so it should fall back to that:

image

And if this was not the case, I don't think this is a "BC Change", this is a "Bug Fix" :D

@trananhmanh89
Copy link
Contributor

trananhmanh89 commented Mar 11, 2021

  1. Exactly, Category Blog has default settings, so it should fall back to that:

image

And if this was not the case, I don't think this is a "BC Change", this is a "Bug Fix" :D

I've found a mistake at my point. That you can change layout of category itself to Listing. In this case, the limit should be global config list_limit as @joomdonation did :).

But if category layout is blog, the limit should respect blog global config, otherwise the pagination will be wrong.

@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Mar 20, 2021
@joomdonation joomdonation deleted the fix_category_articles_list branch May 30, 2021 03:34
@AndySDH
Copy link
Contributor

AndySDH commented Sep 6, 2023

Why was this PR closed, @joomdonation @Quy ?

This is still an issue in Joomla 4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants