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

Make implicit TRUE permissions check explicit in API 4 Explorer for PHP #26514

Conversation

larssandergreen
Copy link
Contributor

Overview

It feels like we end up with quite a few bugs because of the implicit TRUE permissions checks in API 4. I think it might make life a little easier if we make the implicit TRUE explicit in the API 4 Explorer, so if we're copying and pasting we're more likely to stop and think if we should be checking permissions or not.

This only changes the PHP.

The downside is possibly a few more lines of code in core when using traditional style PHP, but that seems well worth it if we avoid a few bugs.

@civibot
Copy link

civibot bot commented Jun 12, 2023

(Standard links)

@civibot civibot bot added the master label Jun 12, 2023
@larssandergreen larssandergreen changed the title Show implicit TRUE Permissions Check in API 4 Explorer for PHP Show implicit TRUE Permissions Check explicit in API 4 Explorer for PHP Jun 12, 2023
@larssandergreen larssandergreen changed the title Show implicit TRUE Permissions Check explicit in API 4 Explorer for PHP Make implicit TRUE permissions check explicit in API 4 Explorer for PHP Jun 12, 2023
@larssandergreen larssandergreen force-pushed the Always-show-implicit-TRUE-Permissions-Check-in-API-4-Explorer branch from a507f95 to 9c02ca3 Compare June 12, 2023 17:26
@colemanw
Copy link
Member

In hindsight, I think the OOP pattern of Civi\Api4\Foo::get(FALSE | TRUE)->execute() is not so great. I wish I had designed it to be more explicit & readable. Maybe like this:

Civi\Api4\Foo::get()
  ->addWhere(...)
  ->executeWithPermissions() // or ->executeWithoutPermissions()

@larssandergreen
Copy link
Contributor Author

@colemanw Yes, that would have been more readable.

@larssandergreen larssandergreen force-pushed the Always-show-implicit-TRUE-Permissions-Check-in-API-4-Explorer branch from 9c02ca3 to 7ce074a Compare June 12, 2023 17:47
@colemanw
Copy link
Member

colemanw commented Jun 12, 2023

@larssandergreen it's kinda not too late to add those 2 functions as wrappers to execute()... but it would kinda be a breaking change because any code using those new functions would be incompatible with earlier versions of civi where they don't exist.
Also, then we'd have 3 ways to do the same thing (there's also the setCheckPermissions(TRUE|FALSE) method). Which feels like adding entropy rather than cleaning anything up.

@larssandergreen
Copy link
Contributor Author

@colemanw We could just use setCheckPermissions(TRUE|FALSE) in the API Explorer instead, definitely more readable, though it's an extra line. What if we showed it as ->setCheckPermissions(TRUE|FALSE)->execute(); (which is kind of the same as you're suggesting).

@colemanw
Copy link
Member

Ugh, the whole reason I added the shorter syntax was because people complained about the wordiness of ->setCheckPermissions(TRUE|FALSE)->execute();.
Can't win 🤷‍♂️

@larssandergreen
Copy link
Contributor Author

I say let 'em complain. If they don't like the output of the API Explorer, they are welcome to use the shorter syntax or the implicit TRUE.

@larssandergreen
Copy link
Contributor Author

But we can also merge this as is, that's fine with me too.

@colemanw colemanw merged commit f113935 into civicrm:master Jun 12, 2023
@colemanw
Copy link
Member

Fine with me too :)

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

Successfully merging this pull request may close these issues.

2 participants