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

Move creating of nav & menu items to legacycustomsearches extension #23862

Merged
merged 4 commits into from
Aug 17, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 21, 2022

Overview

Move creating of nav & menu items to legacycustomsearches extension

Before

Menu & navigation items for legacy custom searches declared in core code

After

Declared in the extension

Technical Details

As part of making it turn-offable....

Comments

@civibot
Copy link

civibot bot commented Jun 21, 2022

(Standard links)

@civibot civibot bot added the master label Jun 21, 2022
'operator' => 'OR',
'separator' => 0,
]);
_legacycustomsearches_civix_navigationMenu($menu);
Copy link
Member

@colemanw colemanw Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI. We have several ways for extensions to add to the nav menu, and none of them are very good:

  1. Insert during installation (hook_civicrm_install) using the api
  2. Use hook_civicrm_navigationMenul
  3. Use managed

They all have downsides. 1 requires manual cleanup during uninstall, and manual handling of multi-domains. 2 is non-configurable, frustrating admins who want to customize their menus. 3 works well as long as you loop through the domains in the .mgd.php. So on balance I think 3 is my favorite, and I've recently started using this pattern:

https://github.com/civicrm/civicrm-core/blob/5bee23815cd56c5214634ee7f40a1b8f52f0ac20/ext/search_kit/managed/Navigation_search_kit.mgd.php

@colemanw
Copy link
Member

@eileenmcnaughton this looks good except for the test failures :/

@eileenmcnaughton
Copy link
Contributor Author

@colemanw I fixed the test failure that seemed to relate - hopefully this will pass now - would be good to get a MOP so I can merge if it does since it now contacts a regen for the sql. Note that the test fails were test-only issues

@colemanw
Copy link
Member

colemanw commented Aug 4, 2022

@eileenmcnaughton still some test issues

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

Successfully merging this pull request may close these issues.

2 participants