@sanderpotjer can you review this please?

@wilsonge sure, will do that in detail tomorrow. But can see already one main issue: all "empty" action values are removed, so

(18, 1, 41, 86, 1, 'com_modules', 'com_modules', '{"core.admin":{"7":1},"core.manage":[],"core.create":[],"core.delete":[],"core.edit":[],"core.edit.state":[]}'),

becomes:

(18, 1, 41, 86, 1, 'com_modules', 'com_modules', '{"core.admin":{"7":1}}'),

This can cause performance issues on sites with a large nested assets table. From my tests (has been a while ago) it seems better to have the empty actions instead of no actions, so the system at leasts knows what actions are available and that no permission is set for an action.

@andrepereiradasilva not sure if you have a specific reason for removing the empty values?

Will this change trigger these notices ? :
#10998

[EDIT]
no, i think remember now,
i believe these notices are triggered if you try to check for

• ACL rule name that does not exist in the access.xml
• and also do not exist in the asset and neither exist in its parent assets

i didn't remove any asset just cleaned the empty (nor allow/nor deny) rules.

i made the changes essencially for sql readability and, without testing, i tought them maybe would be slightly faster.

anyway now i did some tests,

$db = JFactory::getDbo();

$rules = array(
    'com_modules.module.1' => array(
        'old' => '{"core.admin":[],"core.manage":[],"core.create":[],"core.delete":[],"core.edit":[],"core.edit.state":[]}',
        'new' => '{}',
    ),
    'com_content.article.1' => array(
        'old' => '{"core.delete":[],"core.edit":[],"core.edit.state":[]}',
        'new' => '{}',
    ),
);

foreach ($rules as $assetName => $rule)
{
    !JDEBUG ?: JProfiler::getInstance('Application')->mark('<strong>Testing ' . $assetName . '</strong>');

    // Old method: With empty rules.
    JAccess::clearStatics();
    $query = $db->getQuery(true)->insert('#__assets')->columns('rules')->values($rules[$assetName]['old'])->where('name = ' . $db->q($assetName));
    $db->execute();
    $check = JAccess::check($user->id, 'core.edit', $assetName, true);

    // New method: No empty rules.
    JAccess::clearStatics();
    $query = $db->getQuery(true)->insert('#__assets')->columns('rules')->values($rules[$assetName]['new'])->where('name = ' . $db->q($assetName));
    $db->execute();
    $check = JAccess::check($user->id, 'core.edit', $assetName, true);
}

so it actually seems slightly faster.
So, if i'm not missing somthing, it seems to be more or less the same.

@andrepereiradasilva apologises for the delay in my review! Felt of my radar...

I can't find any performance impact with no values set, so it looks like this has been addressed over the past years, which is good news!

The change looks good to me, just a couple very small changes for the com_users.category.77 asset. So thanks for cleaning up the assets data!

@wilsonge my recommendation would be to merge this pull request after the correction. This DOES NOT mean that the whole assets data is correct after that. I do see several inconsistencies, orphan assets and assets missing (like the asset entries for the modules). But is cleans the data up, and could make it easier to further correct the assets data.

I'm happy to do another attempt to provide correct asset data, but think we need some coordination on that to prevent it ends up being outdated and causing merge conflicts a few months later.

@sanderpotjer ok all '' in assets changed to '{}'

I have tested this item ✅ successfully on 84abd14

@andrepereiradasilva thanks for the improvements!

thanks! let's hope this in reviewed/tested/merged faster so i don't need to fix conflicts ...

One question. Are these values consistent with what the ACL API saves to the database? If not, not that I want to discourage this from being merged, but I wouldn't have the base dataset and sample data create records that can't be generated by the API.

@mbabker actually when you create a new article it generates an asset rule in the assets table with the parent (component/category) rules.

Example: {"core.admin":{"7":1},"core.manage":{"6":1},"core.create":{"3":1},"core.delete":[],"core.edit":{"4":1},"core.edit.state":{"5":1},"core.edit.own":[]}

I'm not sure, maybe @sanderpotjer knows better, but actually this seems an issue to me. Because you are hardcoding the component rules in the item asset, which means that if you change the parent (component) rules they will not propagate to the items,

If the API isn't traversing the tree to get the permissions from parent items then that would make sense. It's honestly been a while though since I've done any type of digging into the ACL system though, so I have no idea what it's actually doing.

@mbabker that i know of you create asset rules in 2 scenarios

1. Create a item (press save) you can't change permissions here (because the item is not yet saved ?) so it will inhrit parent rules (why hardcode?)
2. Edit an item that does not have asset in assets table and change permissions via Permission tab, this doesn't hardcode parent rules.

That is indeed a bug!

That is something that is probably caused by the introduction of the AJAX-saving of permissions. If a new articles is being created the actions in the actions should be similar to the "article" section of the access.xml from com_content, so core.delete, core.edit and core.edit.state.

So the default assets that are currently being saved are definitely not correct, but this is a separate issue.

In line with the pull request the default asset should be {} for any new asset, unless permissions are set.

In line with the pull request the default asset should be {} for any new asset, unless permissions are set.

That is what i thought!
I actually think this can make big sites a lot slower because joomla needs to calculate all this hardcoded permissions.

Also since we use ajax now (at least in the core) you cannot (for now) set permissions when creating, only after saving (which also seems a workflow issue to me).

But i agree all of this should be a separete PR, not related to this one.

I have tested this item ✅ successfully on 84abd14

ACL works fine after new install. No missing action values found in assets table.

@brianteeman RTC ?

RTC