User tests: Successful: Unsuccessful:
Pull Request for Issue # .
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)
Make sure the menu items in the sidebar still the same before and after patch
Status | New | ⇒ | Pending |
Category | ⇒ | Modules Administration |
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.
@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.
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.
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).
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.
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.
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.
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.
(Maybe it is a bad idea :D )
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.
Labels |
Added:
?
|
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.
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.
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.
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.
I have tested this item
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.
I have tested this item
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
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:
?
|
Thanks!
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.