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 - Limited support for casting #25595

Merged
merged 3 commits into from
Feb 17, 2023
Merged

Conversation

totten
Copy link
Member

@totten totten commented Feb 16, 2023

Overview

Update the handling of top-level request options (AbstractAction::$foo) so that the magic setter behaves more like a simple setter.

ping @highfalutin cc @colemanw

Before

  • setCheckPermissions(0) casts the 0 to false.
    • This is because it's a concrete setter with explict typing, so PHP rules kick-in.
    • Subsequently, APIv4 validation of $this->checkPermissions passes.
  • setUseTrash(0) does not.
    • This is because it's a magic setter with no typing, so PHP rules don't kick in.
    • Subsequently, APIv4 validation of $this->useTrash fails.

After

  • Both setCheckPermissions(0) and setUseTrash(0) cast the 0 to false.
  • setUseTrash(0) does not support all the same conversion rules as PHP. It just supports the 0/1 rule.

Technical Details

I initially drafted in a way where setUseTrash() performed exactly the same casting as setCheckPermissions(), but there was a countervailing test to assert that setDebug('debug') is invalid, and that seemed fair.

Before
------

* `setCheckPermissions(0)` casts the `0` to `false`.
     * This is because it's a concrete setter with explict typing.
* `setUseTrash(0)` does not.
     * This is because it's a magic setter with no typing.

After
-----

* Both `setCheckPermissions(0)` and `setUseTrash(0)` cast the `0` to `false`

Technical Details
-----------------

I initially drafted in a way where `setUseTrash()` performed exactly the same casting
as `setCheckPermissions()`, but there was a countervailing test to assert that
`setDebug('debug')` is invalid, and that seemed fair.
@civibot
Copy link

civibot bot commented Feb 16, 2023

(Standard links)

@civibot civibot bot added the master label Feb 16, 2023
@highfalutin
Copy link
Contributor

highfalutin commented Feb 16, 2023

Thanks for jumping on this @totten. So, to be explicit about it, we're not accepting string 'true' or string 'false' -- can we make that clear in the cv help? see my docs pr

@highfalutin
Copy link
Contributor

@colemanw
Copy link
Member

Just pointing out that this whole issue would be moot if we bump our min php to 7.4 and can add type hints to class vars.

@totten totten merged commit c9d58b8 into civicrm:master Feb 17, 2023
@totten totten deleted the master-api4-cast branch February 17, 2023 06:44
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.

4 participants