Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Joomla 3.8.0: Disabled com_fields displayed in admin menu #18134

Merged
merged 4 commits into from
Sep 30, 2017
Merged

Joomla 3.8.0: Disabled com_fields displayed in admin menu #18134

merged 4 commits into from
Sep 30, 2017

Conversation

n3t
Copy link
Contributor

@n3t n3t commented Sep 26, 2017

Pull Request for Issue #18046 .

Summary of Changes

Added check to JAdminMenuCSS for disabled or not installed components.

Testing Instructions

Disable com_fields, and see the menu. Links to fields components are present, leading to 404 page.
Apply this patch, and see the menu again.

n3t added 4 commits May 5, 2017 00:42
Joomla 3.8.0 displays com_fields link in menu, ven when com_fields is disabled.
@n3t n3t changed the title Admin menu Joomla 3.8.0: Disabled com_fields displayed in admin menu Sep 26, 2017
@waader
Copy link
Contributor

waader commented Sep 26, 2017

I have tested this item ✅ successfully on 0e0bdbf

Thanks n3t, works great!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18134.

@wojsmol
Copy link
Contributor

wojsmol commented Sep 27, 2017

@izharaazmi Please test this if you have a moment.

@izharaazmi
Copy link
Contributor

I have tested this item ✅ successfully on 0e0bdbf


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18134.

@ghost
Copy link

ghost commented Sep 27, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 27, 2017
@mbabker mbabker added this to the Joomla 3.8.1 milestone Sep 30, 2017
@mbabker mbabker merged commit eb7ba02 into joomla:staging Sep 30, 2017
@joomla-cms-bot joomla-cms-bot added PR-staging and removed RTC This Pull Request is Ready To Commit labels Sep 30, 2017
@n3t n3t deleted the admin-menu branch October 2, 2017 11:19
@infograf768
Copy link
Member

The problem with this code is that it increases drastically the queries.
Any better way? May be by specifically targeting com_fields ?

@izharaazmi
Copy link
Contributor

@infograf768 What do you mean?

@alikon
Copy link
Contributor

alikon commented Oct 7, 2017

@izharaazmi see #18264

@infograf768
Copy link
Member

I guess we could have done

			// Exclude item if com_fields component is not installed or disabled
			if ($item->element == 'com_fields' && (!JComponentHelper::isInstalled($item->element) || !JComponentHelper::isEnabled($item->element)))
			{
				continue;
			}

@izharaazmi
Copy link
Contributor

Your suggestion only checks for com_fields, however the current code checks for all components and excludes a menu item if the related component is disabled/not installed.

@infograf768
Copy link
Member

@izharaazmi
I tested disabling some components like com_associations, com_banners, etc., and it works fine with that code.
I suggest you test yourself.
com_fields is a specific case.

@izharaazmi
Copy link
Contributor

Ok, I'll test it

@izharaazmi
Copy link
Contributor

Okay. I tested it. The checks for items under component container are done internally so they are not needed, fine. For core (protected) components they cannot be disabled from UI, fine.

If we do have any other situation with any components we need this test for them. One such is com_fields. Since a custom menu can have any components link anywhere they will need this tests I think.

@infograf768 I think we must work to minimise the duplicate queries but restricting to com_fields does not seem right. What do you suggest?

@infograf768
Copy link
Member

@izharaazmi
I suggest we discuss on #18264.
See my comment there #18264 (comment)
as I think that we may not really need to check if a comp is installed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants