APiv4 - Add Extension.get

[colemanw]

Overview

Ports the Extension.get API to v4.

Before

Extension API in v3 only.

After

APIv4 now exists for Extensions, but only get action so far.

Comments

I've learned that adding anything non-essential to a new API can bite you in the butt later. So this new api only supports the 'get' action as a first step, and that action returns a limited subset of what's returned by CRM_Extension_System::createExtendedInfo - just the stuff that's really useful and not likely to be deprecated anytime soon.

This new API returns only the short version of the key (e.g. search_kit not org.civicrm.search_kit), as the long version is deprecated we might as well omit it altogether.

I wasn't entirely sure what to do with the name and label properties. It seems like the intention was that label would be a localized version of the name, but I don't see any code in the extension system to actually translate the string to localize the label, it appears to be copied from name as-is:

civicrm-core/CRM/Extension/Info.php

Line 182 in fa3fdeb

$this->label = (string) $info->name;

Nevertheless this new API dutifully returns label (but not name, because it's redundant and likely to get confused with key). IDK maybe someday labels will get translated?

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

I had understood it as not that the key/full_name field was deprecated, but that using dots was discouraged, partly because the script that made a gitlab repo out of the drupal node had problems with dots (or something), and partly for reasons I didn't really follow.

[demeritcowboy]

To expand on what I mean, if the field were deprecated, I should be able to remove it from info.xml. If I do that, the extension won't install. It also doesn't recognize when there are upgrades because the extension feed uses key/full_name. Also

• civix still generates it, and its help text just says the base part of it needs to be unique, but requires a key
• The docs all still refer to it with no mention of deprecation
• The help text at https://civicrm.org/node/add/extension calls it a "recommendation".
• fetch-universe uses it
• probably more?

It might be easier to get rid of file if there's a need to get rid of something, because it's based on key when civix makes it.

[colemanw]

I think @totten should weigh in here, but my understanding is that using dots in the key is deprecated, and if you don't use dots then key === file. The Extension system is getting to the point of ignoring the dots. For example cv en search_kit is now interchangeable with cv en org.civicrm.search_kit. So I thought the forward-thinking approach here would be for APIv4 to return key without the dots.

[totten]

I agree with @demeritcowboy. The distinction between key= and <file> is live in the current ecosystem. "Deprecated" suggests a campaign to remove/replace -- which would impact existing extensions. There are a whole lot of extensions and APIs that use dots. (In my copy of universe, it's 430/650=66% of published exts.) Each of them has a lot of touchpoints that use the dotted name (Civi::resources(), ts(), Transifex, hook_managed, cv en, etc). Changing sounds expensive to me...

I share your preference for key===file (ie skip the dots) -- and I think that's a common preference among active devs. We could probably do more to encourage key===file, eg

• Update dev-docs, civix, and cv to include alerts suggesting that "Your life will be simpler without dots!"
• Provide documentation, tooling, a verified process for how to rename/migrate an extension.
• Encourage using APIs that hide the difference (eg E::path() vs Civi::resources()->getPath(); eg E::ts() vs ts(...domain))
• Audit universe, reduce conflicts, notify folks with non-matching names

For example cv en search_kit is now interchangeable with cv en org.civicrm.search_kit.

Right, it matches on either full_name (<extension key=...>) or name (<file>). Somewhat like "Quick Search" which matches first name or last name. To do this, it needs to see both names.

IMHO, for a data model to accurately describe the Civi 5.x world (ie to be useful for the range of integration scenarios that are likely to come up), it would use both names...

[colemanw]

Ok, so @totten in concrete terms, what do you think the output of this API should be? Currently the fields are

• key
• label
• tags
• description
• version
• status

[totten]

Re: label/name -- Oof, yeah, there are several authorities, and they all disagree (slightly) with each other on terminology (name vs label; key vs full_name vs LONG_NAME). Avoiding the bare-word name does seem like a good idea.

To my eye, key and file have the strongest precedent. (Those are the terms in info.xml, CRM_Extension_Info, and APIv3. The civicrm_extension table half-complies... it uses full_name+file). If I'd implemented this API, I probably would've done key+file without thinking about it.

Example:

But looking at that... I sorta see where you're coming from. The above subtly says that the long version (key) is a proper identifier while the short version (file) is some internal detail (a filename), which maybe seems backwards if you're trying to encourage short identifiers.

If you want to encourage people to think of the short identifier as being equally legit.... there's a little bit of precedent for short_name/long_name. (E::SHORT_NAME / E::LONG_NAME). Maybe this is less biased toward the long name?

[totten]

Tangentially: totten/civix#236 proposes a warning if you have a dot in a new extension-key.

[colemanw]

@totten right. I think my only problem with key + file is that the word "file" implies something internal when it's actually a unique identifier.

However, key + file is the most consistent with the structure of info.xml and the most compatible with APIv3.

The problem with using name (or variations of it like short/long_name) is that it's inconsistent with the info.xml in which <name> actually means "untranslated user-facing label".

So now I'm leaning toward your first impulse of key + file, with some added description in the APIv4 getFields describing file as the unique identifier of choice, and some metadata noting file as the primary key.

[totten]

@colemanw OK,key+file. I've pushed up a patch to use that.

A couple other thing I noticed in r-running:

• You talked about wanting to keep the surface-area minimal. It seems to pass through all fields from createExtendedInfo(). Perhaps that should be a bit more filtered, eg (untested pseudocode):

  static $validFields = static::getFields(0)->execute()->column('name');
  
  $info = \CRM_Extension_System::createExtendedInfo($obj);
  $result[] = CRM_Utils_Array::subset($info, $validFields);

• I was able to use the API explorer to do some queries - eg filter on tags. That was neat. I didn't see a way to exclude mgmt:hidden (eg "where tags does not contain mgmt:hidden). I imagine that's a pre-existing limitation -- and not a blocker for this improvement. (Just worth a note.)

[colemanw]

It seems to pass through all fields from createExtendedInfo().

It seems to at a glance, but it doesn't. Under the hood, the APIv4 BasicGet class already does what you proposed and excludes everything not declared in getFields. That's by design, so that each API doesn't have to do so on an ad-hoc basis. Note the return values in the screenshot in the description.

I didn't see a way to exclude mgmt:hidden (eg "where tags does not contain mgmt:hidden).

Yes, you can do it with the NOT operator. I've updated the screenshot in the description with an example which excludes mgmt:hidden.

[colemanw]

@totten @demeritcowboy I'm happy with this if you're ready to merge it.

[demeritcowboy]

I'm good. Thanks all.

[totten]

It seems to at a glance, but it doesn't. Under the hood, the APIv4 BasicGet class already does what you proposed and excludes everything not declared in getFields. That's by design, so that each API doesn't have to do so on an ad-hoc basis. Note the return values in the screenshot in the description.

Hmm... requires is not in the field list, but - when I ran it - it was happy to return requires. This was the first one I tried it:

It also filters on requires. This other example needs cv@master to execute, but I turned on -v to show the API inputs:

[colemanw]

@totten interesting. I hadn't tried it on the CLI. The APIv4 Explorer won't allow you to select unknown fields.
I've squashed your commit into mine, and pushed up a commit to filter out unknown fields in the BasicGetAction.
While testing I found another bug on the CLI caused by unnecessary permission checks when looking up field options internally, which caused this to fail:

cv api4 Extension.get +s key,status:label

Fixed with 6d0b4f6

[totten]

@colemanw FWIW, almost all of those errors are reporting the same thing:

Exception: Api4 Setting set does not exist.

/home/jenkins/bknix-dfl/build/core-22754-751gs/web/sites/all/modules/civicrm/Civi/Test/Api3TestTrait.php:497
/home/jenkins/bknix-dfl/build/core-22754-751gs/web/sites/all/modules/civicrm/Civi/Test/Api3TestTrait.php:290
/home/jenkins/bknix-dfl/build/core-22754-751gs/web/sites/all/modules/civicrm/Civi/Test/Api3TestTrait.php:173

I think that... if you're inclined to make this strictness global, then it might be easier to pursue that as a separate PR? The Extension.get PR could go ahead with a smaller commit-list (and accept, for now, it's possible to request hidden fields)?

[colemanw]

That's fine @totten I've rebased this to just the one commit and will follow up.
A bunch of test failures were caused by this #22776 so it'd be great to get MOP on this one and that one as well.