@sanderpotjer please also check this

@andrepereiradasilva
I will try to make a through test of this tomorrow (please wait for my test too before merging this)

another note, this one relative to performance, you will notice if you go to, for instance, com_modules options there are less queries (even with super user), with a non super user you will notice even more differences.

Before (30 queries)

After (25 queries)

I have tested this item ✅ successfully on 1f7b8fc

Before:
53 Queries Logged 33.70 ms

After:
51 Queries Logged 30.68 ms

I can see a double:
Application: Before JAccess::getAssetRules (id:55 name:com_kunena)
Application: After JAccess::getAssetRules (id:55 name:com_kunena)

yes, and should work like that, but you can see by the time taken that's it's using several layers of memory caching, no db access.

Also, probably in the code you are making two ACL checks for that component, so you get two checks there.

I have tested this item ✅ successfully on 1f7b8fc

I have tested with Article, Categories, Modules, and my own component, all works as it was in 3.6
@andrepereiradasilva thanks!

I don't think this is B/C with classes extending JAccess.

@phproberto if this PR is not B/C for that motive, then i guess the other PR (#12028 - already merged for 3.7.0) would not be B/C too and, if so, needs to be reverted.

mantainers please advice

RTC

@brianteeman did you read @phproberto comment about potencial B/C issues?

also @ggppdk wants to test this, and i prefer this to be tested by as many people as possible.

so please remove the RTC for now

@phproberto can you give an example or is this just a "theory"

It's just a theory that says that nobody cares about B/C here.

If you cannot see that methods are returning different values, properties are removed, etc. And then we wonder why extension developers create their own framework for everything.

Keep ignoring people and merge this. You have your 2 tests and we are saving those incredible 3ms. That's more important than the CMS reputation.

I compared with J3.6.4 (do not compare with current staging)

• no method signature changes, neither of public nor of protected methods
• 1 protected property removed: protected static $assetPermissionsParentIdMapping = array(); , this variable contains the same data as $assetPermissionsById maybe contructor can make it be an alias to the other variable
• 1 protected method removed: protected static function &preloadPermissionsParentIdMapping($assetType)
• Method protected static function preloadComponents() used to return $components, but no longer returns anything

@andrepereiradasilva

I think you can let these the property and the method in the code,
and mark them Deprecated,

in the rare case that someone both extends the JAccess class, and also tries to use / override the removed method / property
and also you can make preloadComponents() also return $components as before ?

I have tested a little, i will test properly later today and post my test too

[EDIT]
also propected property that is removed: $assetRulesIdentities
and propected property: $assetRulesIdentities is renamed to: $assetIdByName

plus the returning value preloadPermissions() that @phproberto mentions below

Method preloadPermissions() was returning boolean and now returns void:

https://github.com/joomla/joomla-cms/pull/12809/files#diff-bbdd2441fb63d5dea47802e51fc9df75L297

1 protected property removed: protected static $assetPermissionsParentIdMapping = array();
1 protected method removed: protected static function &preloadPermissionsParentIdMapping($assetType)
Method protected static function preloadComponents() used to return $components, but no longer returns anything

Which means @phproberto is right and this isn't B/C.
It is perfectly fine to extend this class, and thus anything which isn't private has to stay and behave the same.

Removing RTC label.

I agree with @phproberto and @Bakual. This PR breaks B/C. You cannot merge it in 3.x. Any changes which modify public and protected members must only be included in the next major version, 4.0.

It does not matter if extending JAccess is "rare" or not. This project has committed to semantic versioning. As a result any backwards incompatible changes, no matter how "rare" their impact you think it is, cannot go in a minor version. Putting this to discussion or, worse, ignoring it when putting an RTC label reflects very badly on the trustworthiness of this project and the competence of its community. Please be careful with b/c. We don't want to go back to the Wild West of 1.5 to 3.1.

This PR breaks B/C.

yes, but all cases of B/C break can be fixed and then this can be merged (after re-testing)

Please be careful with b/c.

Totally agreed

Nobody here said changes can't be done to JAccess @ggppdk. But they should be done in a B/C way and non-backward compatible changes done in #12028 reverted.

Plus do not ignore people when they invest their time checking code.

We can only review and comment on code written. Tags apply to the code written. If the code is rewritten it will be retested.

That said, my personal take on this is that the b/c changes are the heart of this PR and were made for performance reasons. Reverting them would essentially revert the PR or, worse, end up with dead code in the class not used elsewhere. I'd like to be proven wrong. So write the code and I'll review it. Fair?

Just to say i'm not ignoring anyone. that's why i asked to check b/c.

thanks all for your time and comments. I will make a pr to revert the other PR to solve the current issue and b/c issues. will close this one and will work on a b/c alternative that includes both pr otimizations or will make it to 4.0 branch.

Thank you @andrepereiradasilva! Please tag us who commented here on your new PR so we can test it :)

sure, thanks you again

@phproberto sorry you misunderstood what I was trying to say (and its my fault I wrote too short a reply)