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

APIv4 - Treat navigation permissions as array, add pseudoconstant for operator #22160

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Nov 29, 2021

Overview

Adds metadata for the sake of consistently handling the permission and permission_operator fields in APIv4.

Before

API treats comma-separated permission field inconsistently: as string for Navigation entity but as array for Dashboard.
No discoverable options for permission_operator in either entity.

After

Consistent with other serialized strings, the API treats comma-separated permission as an array
Options for permission_operator discoverable.

Technical Details

Technically, this is a breaking API change for the Navigation entity. However I highly doubt anyone is using it in the wild. Even our core code uses APIv3 for nav menu items. I found and updated one line in the SearchKit that was passing a string and now needs to pass an array. Afform, etc. didn't need updating because it still uses v3.

This change doesn't affect v3, which is fairly indifferent to metadata. So if we're going to do this change I'd rather get it over with now before APIv4 managed entities becomes a thing in this (5.45) release cycle.

@civibot
Copy link

civibot bot commented Nov 29, 2021

(Standard links)

@civibot civibot bot added the master label Nov 29, 2021
… operator

This gives consistency in how the fields are handled in the Navigation and Dashboard entities
@eileenmcnaughton
Copy link
Contributor

@colemanw I agree with this standardisation and I think it is low-risk from a compatibility point of view - but does require documentation in our api change log

@colemanw
Copy link
Member Author

Ok @eileenmcnaughton you twisted my arm and I've spent some time cleaning up https://lab.civicrm.org/documentation/docs/dev/-/blob/master/docs/api/v4/changes.md

I added disclaimer at the top, to say that only "major or breaking changes" will be listed here and other notes found in the following other places...

That makes me feel better about the thing not being too overwhelming, maybe now we (I) can keep up with it.

@colemanw colemanw merged commit ed679e7 into civicrm:master Nov 29, 2021
@colemanw colemanw deleted the navigationPermission branch November 29, 2021 23:02
@eileenmcnaughton
Copy link
Contributor

looks good @colemanw

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