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

Setting Admin - Add mixin to autogenerate a settings form #27569

Merged
merged 6 commits into from Oct 16, 2023

Conversation

totten
Copy link
Member

@totten totten commented Sep 22, 2023

Overview

@mattwire @eileenmcnaughton @seamuslee001 - I was reading https://docs.civicrm.org/dev/en/latest/framework/setting/extension/, and it struck me that many of these steps could be done automatically...

Before

Suppose you are creating extension foobar. It needs some settings and a setting page. Process:

  1. Create the extension (civix generate:module foobar)
  2. Create the settings (nano settings/foobar.setting.php)
  3. Implement hook_civicrm_permission and declare administer foobar
  4. Create xml/Menu/foobar.xml and define route civicrm/admin/setting/foobar. Enable mixin menu-xml@1.
  5. Update each setting to declare settings_page => ['foobar' => ['weight' => 123]] (with differing values of 123)

(And, of course, clear caches.)

After

The last several steps are consolidated into one step. Process:

  1. Create the extension (civix generate:module foobar)
  2. Create the settings (nano settings/foobar.setting.php)
  3. In info.xml, add <mixin>setting-admin@1</mixin>.

(And, of course, clear caches.)

Comments

Example extension: https://lab.civicrm.org/-/snippets/95

This mixin should be amenable to backporting. So we could add it to civix and use it for extensions that run on older versions of civicrm-core. I don't know how far back compatibility would go... but probably several years. For DX, the process would be like:

$ civix mixin --enable=setting-admin@1,setting-php@1
Updating info.xml
Writing mixin/setting-admin@1.0.0.mixin.php
Writing mixin/setting-php@1.0.0.mixin.php

If the concept seems good, then we should add some tests to the mixin.

@civibot
Copy link

civibot bot commented Sep 22, 2023

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@eileenmcnaughton
Copy link
Contributor

@totten yep - looks good!

@mattwire
Copy link
Contributor

So the path is always the extension key? Sounds reasonable just not clear from description.

I'm so used to "wiring" up extension settings pages I hardly think about it but it's a lot of steps and this looks really nice + simplifies extension + makes a transition to a non-quickform settings page easy in the future (ie. just replace the mixin).

@totten
Copy link
Member Author

totten commented Sep 23, 2023

So the path is always the extension key? Sounds reasonable just not clear from description.

Yeah, I had done the path with the key (long name). But everything else used the file (short name). And I think the short name is probably more typical of existing/real-world settings-pages. So I'll switch it to match.

@totten totten force-pushed the master-setting-admin branch 2 times, most recently from 71a45fe to 62c9710 Compare September 25, 2023 23:23
@totten totten changed the title Setting Admin - Proof-of-concept for mixin/setting-admin@1 Setting Admin - Add mixin to autogenerate a settings form Sep 26, 2023
@totten
Copy link
Member Author

totten commented Sep 26, 2023

OK, multiple updates:

  • Standardized on the short-name (eg myext instead oforg.example.myext) for paths/permissions/etc.
  • Added E2E tests
  • Added nav-link
  • Converted a couple of core-exts (authx and recaptcha) to use it
  • Did some light r-run in other languages. (The page-titles -- "Authentication" and "reCAPTCHA Settings" -- don't appear to be in my l10n/fr_CA data. But if I hack info.xml and use some well-known strings, then I can see that the ts() mechanism is being applied - same as it was before the patch.)
  • Resolved MC from recent PR

* @return string|null
* Return NULL by default. If the walk was short-circuited, then return that value.
*/
public static function walk(&$items, callable $callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this like run but slower?

Copy link
Member Author

Choose a reason for hiding this comment

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

rofl. Yeah, basically.

FWIW, for an example of this wording, see https://www.php.net/array_walk or https://www.php.net/array_walk_recursive. It's the same basic idea, except that it's tuned to the properties used by nav-menu (e.g. $item, $item['child'], $item['attribute']).

@mattwire mattwire merged commit ae620a4 into civicrm:master Oct 16, 2023
3 checks passed
@totten totten deleted the master-setting-admin branch October 19, 2023 21:18
@highfalutin
Copy link
Contributor

Is there a docs MR for this?

The relevant docs page looks to be Creating a extension setting and admin form and text could be borrowed from the code doc

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