Hi

1. About category view / multi-article listings

You can not apply such changes to the loop of get items
you are removing items after the query, which has 3 problems

• you will make pagination look broken , some pages will have 3 items some will have 10
• even if pagination is off you will show a random number of items into the category view / multi-article listings
• you are slowing down the view when retrieving a large number of items e.g. 50 - 500 items

the decision to include or exclude published items must be part of the query

Now about the existing approach of using the

if ((!$user->authorise('core.edit.state', 'com_content')) && (!$user->authorise('core.edit', 'com_content')))

i agree the above is not so good either because having edit/edit.state at component level but not having edit/edit.state e.g. in a category AA , will allow someone to list articles of category AA that are not published

my approach (in our component) is always to filter out unpublished items, and use 1 extra ACL rule, only at component level to allow listing unpublished items (or if user is not guest and is owner of the items)

in any case ACL checks per item in the item loop after the query are not good option for the reasons stated above

2. About retrieving a single record , then yes you can apply the changes that you did
   allowing viewing unpublished if user has edit / edit.state at the particular article asset is correct

you are fixing an existing approach,

Now to comment on the existing approach

existing approach is not proper, it is not proper to do such check at DB model

• the DB model should return the item and the controller or the view should decide to how to use it , show it or not
• it is OK if view / controller will call a method of the DB model to get view/edit/etc access, so i do not say that having ACL calculation in the DB model is bad, the bad thing is to use it inside the DB model the way that it is used

e.g. on some page visit, some plugin is triggered to do some work
and loads the article DB model to do it
now because DB model will not return the unpublished item such work can not be done
ok and the plugin has to use direct SQL queries to do the work

I argue that DB model should only have methods to calculate access view / edit / etc
but it should not make use of them

as said above my point 2 is not relevant to your PR , for number 2 you are improving the existing approach

• **my suggestion is that it should be left to the caller to decide if
  getItem() needs to check view publication state (and bypass publication state according to ACL edit/edit.state on the item) via a parameter that will be given to getItem() !
  now such option does not exist

About 1.

Slowing down: the performance penalty can came only from:

• the foreach loop at the end of getItems function that removes articles from $items array or
• the different authorisation: at article level than at component level.
  In fact authorisations and selections are the same done at the moment by J!3.7.5.
  How much do you think would be the percentual penalization?

Pagination: Oh! Pagination! Right.

About 2.
Thanks. Seems resonable.
Ok, we could move the authorisation and 'filtering' logic to view, but how to solve the pagination problem? (I think it will remain)
Or could we simply display empty placeholders and a message like "Some elements may have been hidden as you have not permissions to display them." ?

@LivioCavallo

besides the pagination problem i also mentioned and the slow down
there is 3rd thing that i mentioned:

even if pagination is off you will show a random number of items into the category view / multi-article listings

so i say let's not change at all the getItems method

you can make your PR just for the case of single item
with 1 modification for new records $pk is zero , do not try to check ACL on asset:
com_content.article.0

Do you have a measure or an estimate of the slow down?

Calling authorise on 'com_content.article.0' simply returns false when needed.
Moreover article model is not used for new records, in that case J! uses the form model, then article model will never be passed a $pk value of 0.

I cannot reproduce your problem with random items in category view / multi-article listings. When does it happen?

Yes, we now have PR #17735 for single article and this for featured and article listings.

hi, lets forget the slow down totally ! (when listing more than 50 or more than 100 items)

and focus on the 2 bugs that this PR introduces

• you are breaking pagination, some pages will have 3 items some will have 10
• even if pagination is off you will show a random number of items into the category view / multi-article listings

Any checks of which items to include

• must be done / decided before the SQL query items

e.g. you can fix your reported issue by another way
suggest a PR that unpublished items will be shown to

• super-users
• item owners

or you can introduce a new ACL rule (component level only) "Ignore publication status"
the above are just some ideas

@LivioCavallo are the Bugs mentioned by @ggppdk are solved and this PR is good for Test?

Sorry that it has taken so long to respond to this pull request. Unfortunately this pr has unknown side effects, especially with pagination. In the current state it is not something we can consider to get merged, so I'm closing it. If you still see it as an issue, then please rebase to the 4.2 branch. For the testers, please do some proper testing with front end list views and pagination. Also consider to add some system tests for API testing pagination. Thanks for your contribution and help, making Joomla better.