? Pending

User tests: Successful: Unsuccessful:

avatar sawmurai
sawmurai
27 Feb 2021

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

avatar sawmurai sawmurai - open - 27 Feb 2021
avatar sawmurai sawmurai - change - 27 Feb 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 27 Feb 2021
Category Modules Administration
avatar sawmurai sawmurai - change - 27 Feb 2021
Labels Added: ?
avatar sawmurai sawmurai - change - 27 Feb 2021
The description was changed
avatar sawmurai sawmurai - edited - 27 Feb 2021
avatar bembelimen
bembelimen - comment - 27 Feb 2021

@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.

avatar richard67
richard67 - comment - 27 Feb 2021

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:

$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).

  1. 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:

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.

  1. 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 .
avatar richard67
richard67 - comment - 27 Feb 2021

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;
avatar sawmurai
sawmurai - comment - 27 Feb 2021

@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.

avatar alikon
alikon - comment - 1 Mar 2021

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

avatar sawmurai
sawmurai - comment - 1 Mar 2021

Agreed! Did not know about the ComponentHelper

avatar richard67
richard67 - comment - 2 Mar 2021

@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.

avatar sawmurai sawmurai - change - 2 Mar 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-03-02 12:32:32
Closed_By sawmurai
avatar sawmurai
sawmurai - comment - 2 Mar 2021

Yep, I will close it :)

avatar sawmurai sawmurai - close - 2 Mar 2021

Add a Comment

Login with GitHub to post a comment