? ? Pending

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
18 Jul 2021

Pull Request for Issue # .

Summary of Changes

This PR makes a small modification to administrator mod_menu so that third party extensions (which integrate with Joomla! custom fields) can add links to fields and field groups of com_fields by place these links in component menu like this https://github.com/joomdonation/weblinks/blob/4.0-compatible/src/administrator/components/com_weblinks/weblinks.xml#L55-L58 instead of having to write many code to insert these menu items manually like this block of code https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_admin/script.php#L7485-L7528

(and trust me, it is even more complicated during updating extension)

Testing Instructions

Test 1

Make sure the menu items in the sidebar still the same before and after patch

Test 2

  1. Download weblinks package for Joomla 4 at https://github.com/joomla-extensions/weblinks/files/6836285/pkg-weblinks-4.0.0-dev.zip, install it
  2. Access to Weblinks -> Links, click on Options button in the toolbar. Go to Integration tab, set Edit Custom Fields parameter to No
  3. Before patch, you always see Fields and Field Groups menu links under Weblinks
    fields_and_fields_groups
  4. After patch, the Fields and Field Groups menu items will be removed when you set the parameter to No. When you set the option back to Yes, these menu links are visible again.
avatar joomdonation joomdonation - open - 18 Jul 2021
avatar joomdonation joomdonation - change - 18 Jul 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 18 Jul 2021
Category Modules Administration
avatar joomdonation
joomdonation - comment - 18 Jul 2021

To be more clear, there is no problem with add these links to component sub-menus. The problem here is that without this change, these sub-menu items won't be removed when Custom Fields Integration is disabled in the component.

avatar Bakual
Bakual - comment - 18 Jul 2021

Isn't the real issue here that $item->element isn't properly populated? The code regarding categories a few lines above doesn't work properly as well due to this.
So I'd say it would make more sense to fix that code part. Or rewrite it to not check the $item->element but the option of the link?
Unfortunately I haven't found yet where that element is coming from and why it is correctly populated for contacts but not for my own component.

avatar joomdonation
joomdonation - comment - 18 Jul 2021

@Bakual The data for element comes from SQL, join #__menu table with #__extensions table base on data from component_id field in #__menu table.

So without this modification, somehow in your component install script, you have to update component_id field in #__menu table for these links with ID of com_fields component (It is populated with ID of your own component by default).

Hard to explain but it is complicated, especially during update component process. So the modification here is the easiest option I found.

avatar Bakual
Bakual - comment - 18 Jul 2021

I found it now as well. And I actually think the checks are done wrong. It should never check $item->element because the menuitems correctly are set to the extension ID of my component (the core ones actually are done wrong), as they belong to my extension.
The checks (probably all in that if-elseif-else stuff) instead should check the option part of the link instead, because that is actually what it is looking for.

avatar Bakual
Bakual - comment - 18 Jul 2021

Or if the element is indeed supposed to be the referenced extension, then it should be fixed at the stage where those menuitems are registered (during extension installation).

avatar joomdonation
joomdonation - comment - 18 Jul 2021

it should be fixed at the stage where those menuitems are registered (during extension installation).

Afraid of it is complicated (have tried it with com_weblinks). So the check like I modified here seems to be fine (we can even remove condition $item->element === 'com_fields' if needed. I leave it there to be safe only.

avatar Bakual
Bakual - comment - 18 Jul 2021

My point is that the other checks don't work as well. So it would be more useful if we extract the option from the link (eg using URI) prior to the checks and then work with that. Eg the cpanel and com_categories checks also fail for the same reason as the com_fields one.
But if you want to restrict the fix to this particular issue, it's fine. Maybe I find time to expand it later.

avatar joomdonation
joomdonation - comment - 18 Jul 2021

I didn't look at and didn't know that other checks don't work, too. I will try to look at it again on tomorrow, then.

avatar joomdonation
joomdonation - comment - 18 Jul 2021

Another solution would be changing ComponentAdapter _buildAdminMenus function to allow passing the component for the menu item

So for the links to com_fields like in this example, we can add new attribute component="com_fields" in the menu item definition. Same for link to com_categories. We can store correct data for component_id find in #__menu table during installation process and do not have to change anything here.

https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Installer/Adapter/ComponentAdapter.php#L1108

(Maybe it is a bad idea :D )

avatar Bakual
Bakual - comment - 18 Jul 2021

You wouldn't need a new attribute. You could just parse the link there as well and take the option from it. That should work for all cases imho.

avatar joomdonation joomdonation - change - 19 Jul 2021
Labels Added: ?
avatar joomdonation
joomdonation - comment - 19 Jul 2021

Look like changing _buildAdminMenus in ComponentAdapter is not a safe method because we remove existing menu items during the build menu process base on ID of the installing/updating component. So if we update component_id for these links to ID of the real component (instead of ID of our own component), these menu items will be left and we will get SQL error during the update (I don't remember exactly the error message, but it would happpen)

So for now, I changed the logic to check the actual component passed in the menu item link. That would solve problem with menu items links to com_categories from third party components, too

@Bakual Could you please take a look at the proposed code to see if it is OK ? If this is accepted, similar change will need to be done for mod_submenu, too.

avatar Bakual
Bakual - comment - 19 Jul 2021

Look like changing _buildAdminMenus in ComponentAdapter is not a safe method because we remove existing menu items during the build menu process base on ID of the installing/updating component. So if we update component_id for these links to ID of the real component (instead of ID of our own component), these menu items will be left and we will get SQL error during the update (I don't remember exactly the error message, but it would happpen)

So the element indeed should be the ID of the component to which the menuitem belongs to, and not like in core where the menuitem points to ?

Could you please take a look at the proposed code to see if it is OK ? If this is accepted, similar change will need to be done for mod_submenu, too.

From looking at the code, this should work in a B/C way, yes.

avatar joomdonation
joomdonation - comment - 19 Jul 2021

So the element indeed should be the ID of the component to which the menuitem belongs to, and not like in core where the menuitem points to

Right. Good thing is that for core components, we do not have to care about un-install and re-install or update... So we can live with that bugs :D

From looking at the code, this should work in a B/C way, yes.

Guess we need your help to test it with your extensions and confirm that it is working as expected.

avatar joomdonation joomdonation - change - 19 Jul 2021
The description was changed
avatar joomdonation joomdonation - edited - 19 Jul 2021
avatar Bakual
Bakual - comment - 19 Jul 2021

I'll try to find time to test it this evening. If someone else want to try with my component, I've just created a new beta release which should work for this: https://github.com/Bakual/SermonSpeaker/releases/download/6.0.0-beta3/com_sermonspeaker.zip
Edit: Sorry, I need to first add that parameter. As it was useless so far, I haven't included it yet.

avatar Bakual Bakual - test_item - 19 Jul 2021 - Tested successfully
avatar Bakual
Bakual - comment - 19 Jul 2021

I have tested this item successfully on 51fc127


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

avatar Bakual
Bakual - comment - 19 Jul 2021

I've added the config option now to my component. If someone wants to try it, the option is in the "Integration" tab and it will toggle the appearance of the "Fields" and "Field Groups" menuitems.
com_sermonspeaker.zip

PR also works if the component com_fields is disabled. The links are removed then as well. I haven't tested the access permission parts.

avatar alikon alikon - test_item - 21 Jul 2021 - Tested successfully
avatar alikon
alikon - comment - 21 Jul 2021

I have tested this item successfully on 51fc127


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

avatar alikon
alikon - comment - 21 Jul 2021

I have tested this item successfully on 51fc127


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

avatar alikon alikon - change - 21 Jul 2021
Status Pending Ready to Commit
avatar alikon
alikon - comment - 21 Jul 2021

RTC


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

avatar wilsonge wilsonge - change - 21 Jul 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-07-21 21:42:16
Closed_By wilsonge
Labels Added: ?
avatar wilsonge wilsonge - close - 21 Jul 2021
avatar wilsonge wilsonge - merge - 21 Jul 2021
avatar wilsonge
wilsonge - comment - 21 Jul 2021

Thanks!

Add a Comment

Login with GitHub to post a comment