fix(mod_submenu): hide disabled components in mod_submenu

[sawmurai]

Pull Request for Issue #32456 .

Summary of Changes

The changes introduced hide menu items for disabled components from the cpanel.

Testing Instructions

1. Disable the CSP extension

Actual result BEFORE applying this Pull Request

The menu item "Content Security Policy" is still available in the cpanel.

Expected result AFTER applying this Pull Request

The menu item "Content Security Policy" is not available in the cpanel anymore.

Documentation Changes Required

[bembelimen]

Probably you should set an empty array as default, so it's not braking if some extension using this call...

[sawmurai]

Good idea, thank you!

[bembelimen]

@SharkyKZ could you please elaborate why you gave a thumb down? Probably there is room for improvement in this PR based on you feedback. Thanks.

[richard67]

I don't know if this is the right approach or not, but it contains a few mistakes:

1. The extension name is not unique in the extensions table, so there may be several extensions of different types having the same name value. What has to be unique is the combination of element, folder and client_id for extensions of type "plugin", and element and client_id for extensions of other types. But the database query here doesn't contain any restrictions ("WHERE ...") for the extension type and client_id:

joomla-cms/administrator/modules/mod_submenu/src/Menu/Menu.php

Lines 44 to 46 in 47b2cff

	$query->select($db->quoteName('e.name'))
	->from($db->quoteName('#__extensions', 'e'))
	->where($db->quoteName('e.enabled') . ' = 0');

As far as I understand the purpose of this PR, only extensions of type "component" should be fetched with that query, and they should be restricted also to the right client_id here (1 for admin).

2. The property "element" is checked to be in the array, which contains the "name" values and not the "element" values, as you could see by the query mentioned above. See here the check:

joomla-cms/administrator/modules/mod_submenu/src/Menu/Menu.php

Line 85 in 47b2cff

if ($item->type === 'component' && in_array($item->element, $disabledExtensions, true))

For lucky circumstances, the "name" and "element" values are the same for (core) components, but that might not always be granted, so it can happen we compare apples and pears here. Therefore at least from a formal point of view the above check is wrong.

3. Beside the above 2 points my knowledge about the submenu is too little to say if the approach chosen here is right or wrong. Hoping for more detailed feedback e.g. from @SharkyKZ .

[richard67]

P.S. to my above comment's items 1 and 2: For the reasons stated there, I would expect the query to be:

		$query->select($db->quoteName('e.element'))
			->from($db->quoteName('#__extensions', 'e'))
			->where($db->quoteName('e.type') . ' = ' . $db->quote('component'))
			->where($db->quoteName('e.client_id') . ' = 1')
			->where($db->quoteName('e.enabled') . ' = 0');

And later below it should be:

$names[] = $item->element;

[sawmurai]

@richard67 Thank you so much for the comments! I updated the PR. Let's see what @SharkyKZ's feedback is.

This would be my first contribution to joomla despite having used it for the past couple of years, so I would not be surprised if I misunderstood something and my approach is totally wrong.

The idea behind my approach is basically:
Since the menu is rendered from the preset-xml files (in the error case) my only information are the element and type of the menu item, so I know that its a component of "type" com_csp. To link that to the state of the component (enabled, disabled) I have to query the database beforehand and pass that information to the precheck function, which already appears to be hiding menu items under certain conditions.

[alikon]

humm....
too much code imho see #32555 too...

[sawmurai]

Agreed! Did not know about the ComponentHelper

[richard67]

@sawmurai If you agree that PR #32555 shall replace this PR here: Can we close your PR here? Of course you can do that yourself, too.

[sawmurai]

Yep, I will close it :)