[#44950] - [5.3] Fix item count does not take access level into account
- Pending
- Medium
- Build: 5.2-dev
- # 44950
- Diff
- janschoenherr:patch-30
User tests: Successful: Unsuccessful:
Currently when getting the item count of a category, the access level is not being taken into account. This PR fixes the issue.
Summary of Changes
Add the access condition to the item subquery.
Testing Instructions
Create a menu item of type List All Categories in an Article Category Tree.
If you navigate to the menu item, it should display a list of categories with an article count.
Have at least one article in one of the categories restricted by its access level. Make sure you are not logged in.
Actual result BEFORE applying this Pull Request
The access restricted article is being included in the article count.
The Articles > Unauthorized Links option has no effect on the article count.
Expected result AFTER applying this Pull Request
The access restricted article is not being included in the item count.
If however Articles > Unauthorized Links option is set to Yes the access restricted article is included.
Link to documentations
Please select:
-
Documentation link for docs.joomla.org:
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Labels |
Added:
PR-5.2-dev
|
||
@janschoenherr I think the test instructions belong to the new Articles-module? If so i didn't find where the "Article Count" to set. If not, can you please be more specific in the test instructions?
I updated the testing instructions.
I have tested this item ✅ successfully on dd5e3b4
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
I have tested this item ✅ successfully on dd5e3b4
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
I have tested this item 🔴 unsuccessfully on dd5e3b4
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
This doesnt take into account what happens when you have unauthorised links set to yes
Before the PR the menu item would display the total number of articles and then when entered the category will display the intro text of the registered articles to guest users
After the PR the menu item only displays the public number of articles which if greater than 0 will still display the intro text of the registered articles to guest users. If the number of public articles in the category is 0 then the category will not be displayed at all.
See video
After PR
chrome_RAEUI9rGdb.mp4
Before PR
chrome_04rbtN0569.mp4
There are probably other areas negatively impacted by this change - this was just the first one that came to mind
| Category | Libraries | ⇒ | Front End com_content Libraries Modules |
I have added a new option accessOnItems to resolve that.
$params = (clone $this->getState('params'))->merge($params);
This actually fixes another bug. Just evaluating the menu params, doesn't take the global component parameters into account. e.g. Use Global (hide) Which might lead to fetching countItems unnecessarily.
@janschoenherr Can you update the test instructions including the #44950 (comment) by @brianteeman and your comment?
This actually fixes another bug. Just evaluating the menu params, doesn't take the global component parameters into account. e.g.
Use Global (hide)Which might lead to fetchingcountItemsunnecessarily.
This actually fixes another bug. Just evaluating the menu params, doesn't take the global component parameters into account. e.g. Use Global (hide) Which might lead to fetching countItems unnecessarily.
Regarding my additional comment:
You'll have to enable Debug System.
Then set Empty Categories to Show and # Articles in Category to Hide
in the Global Configuration.
Without the fix, the numItems are still queried:
With the fix, they are not:
I have tested this item ✅ successfully on 5085b03
Unauthorised Links No
Unauthorised Links Yes
- The categories
All articles are publicshow 7 articles andNo articlesshow no articles. No difference what valueUnauthorised Linksis set. - Category
No articlesisn't shown in menu item typeList All Categories in an Article Category Tree. Register to read more …works as expected.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
I have tested this item ✅ successfully on 24db2a0
I have tested this successfully. :)
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
I have tested this item ✅ successfully on 24db2a0
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
| Category | Libraries Front End com_content Modules | ⇒ | Unit Tests Repository Administration com_admin SQL Postgresql com_associations |
| Labels |
Added:
Unit/System Tests
|
||
| Category | Unit Tests Repository Administration com_admin SQL Postgresql com_associations | ⇒ | Front End com_content Libraries Modules |
@janschoenherr - now when I try to test, we have a regression possibly, the results are the same with and without the PR - unlike in my previous test where things were different and matching the aim of the PR (filtering out the item count).... FYI.
| Labels |
Removed:
Unit/System Tests
|
||
@janschoenherr yes, I am using a menu item of type List All Categories in an Article Category Tree... I just retested it, 1 minute ago and still same reaction - the numbers do not move, if need be, I can make a video...
And you are not logged-in in the frontend, right?
And you are not logged-in in the frontend, right?
Definitely only logged into the backend. Not the front-end.
@janschoenherr - Could the issue be that I set those articles to Special and Super Users vs Registered or Publisher etc?
Just changed one of the articles from Special to Registered and it didn't work either ;( - sorry.
I have tested this item ✅ successfully on 0ed6a90
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
I have tested this item ✅ successfully on 0ed6a90
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
Here's a before-and-after look at the results of this PR!
PR.-.44950.2.1.mp4
| Category | Front End com_content Libraries Modules | ⇒ | Unit Tests Repository Administration com_admin SQL Postgresql com_associations |
| Title |
|
||||||
| Category | Unit Tests Repository Administration com_admin SQL Postgresql com_associations | ⇒ | Front End com_content Libraries Modules |
Please fix phpcs: https://ci.joomla.org/joomla/joomla-cms/83956/1/7
| Labels |
Added:
PR-5.3-dev
|
||
I have tested this item ✅ successfully on cc910c8
I have tested this successfully this time :D.
@janschoenherr (sorry it took me so long to re-test this).
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
Thanks for testing again!
I have tested this item ✅ successfully on 5aabb98
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
I have tested this item ✅ successfully on 5aabb98
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44950.
This is not correct/complete please do not merge
| Labels |
Added:
Updates Requested
PBF
Removed: PR-5.2-dev |
||
This is not correct/complete please do not merge
@brianteeman Anything else missing besides the limitation to core components, which has been implemented with the last commit?
This PR only introduces the changed counting for com_content (even though the problem exists in anything using categories) but there is a restriction placed in a library file for categories to just core extensions.
@brianteeman The other two core extensions com_contact and com_newsfeed do not have a 'show_noauth' option and therefore will filter items by default now.
even worse then - if it is only for content categories why touch the generic category library at all
No, with this PR the item count of the other two components are correctly filtered by access now. Previously they weren't.
so why restrict it to core extensions then
Because tables of 3rd party extensions might not have a access column and the query would fail for them (That would be a breaking change). 3rd party extensions need to set the option explicitly.
then how can they do that as you are restricting this to core extensions. Surely it is just better to check if the extension using categories have an access column than to exclude them all on the basis that one might not
I think that's an misunderstanding.
$options['accessOnItems'] ??= ExtensionHelper::checkIfCoreExtension('component', $this->_extension, 1);
The ??= operator basically says: 'If you are not set, please use the value to the right'.
Anyway, I just came up with a better approach. I will update the PR in a little while.
| Category | Front End com_content Libraries Modules | ⇒ | Front End com_contact com_content com_newsfeeds Libraries Modules |
| Category | Front End com_content Libraries Modules com_contact com_newsfeeds | ⇒ | Front End com_contact com_content com_newsfeeds Libraries |
The new approach sounds much better. Will check when I get back to my pc tomorrow
@janschoenherr I think the test instructions belong to the new Articles-module? If so i didn't find where the "Article Count" to set. If not, can you please be more specific in the test instructions?