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

Add string type declarations to global API functions. #15864

Merged
merged 2 commits into from
Nov 20, 2019

Conversation

mfb
Copy link
Contributor

@mfb mfb commented Nov 15, 2019

Overview

As of PHP 7 we can add string type declarations.

Before

These functions have only type hints for the first two params.

After

The string params are also enforced via type declarations.

@civibot
Copy link

civibot bot commented Nov 15, 2019

(Standard links)

@civibot civibot bot added the master label Nov 15, 2019
@totten
Copy link
Member

totten commented Nov 15, 2019

Yup, makes sense to me. 👍

(Tangent: One might be reserved about filling-in signatures generally - e.g. one might say array when a better type woud be Traverseable. But this change makes a lot of sense -- seems manifest that civicrm_api() needs the $entity and $action to be either string or readily-convertible-to-string.)

@totten totten added the merge ready PR will be merged after a few days if there are no objections label Nov 15, 2019
@mfb
Copy link
Contributor Author

mfb commented Nov 16, 2019

The v3 API explicitly checks if params is an array, not just iterable. The v4 API might work with iterable though? I didn't try this

@mfb
Copy link
Contributor Author

mfb commented Nov 16, 2019

Looks like we can't use iterable as a type declaration until PHP 7.1 is the minimum version.

@eileenmcnaughton
Copy link
Contributor

test this please

@totten
Copy link
Member

totten commented Nov 18, 2019

For the array constraint - this PR doesn't change that, so I'm +1 on the PR anyway.

The weird r-test thing is.... the test timed out in both previous runs for this patch (30630, 30609).

ok 1840 - CRM_Utils_API_MatchOptionTest::testCreateMatch_one with data set #1 ('match-mandatory')
ok 1841 - CRM_Utils_API_MatchOptionTest::testCreateMatch_many with data set #0 ('match')
ok 1842 - CRM_Utils_API_MatchOptionTest::testCreateMatch_many with data set #1 ('match-mandatory')
not ok 1843 - Error: CRM_Utils_API_MatchOptionTest::testReplaceMatch_Email
not ok 1844 - Error: CRM_Utils_API_MatchOptionTest::testReplaceMatch_Address
Build timed out (after 60 minutes). Marking the build as aborted.
Terminated
Build was aborted

It feels like this might be some weird/quirky thing specific to this change. I can't see any other/similar timeouts among recent PR tests. The timeout happened in the same spot in both runs. (Background: The timeout policy is this: if the build-job doesn't produce any console output for 60m, then it aborts. So in both cases, it spent 60m with no output after testReplaceMatch_Address.)

Let's give it one more test-run to rule out some fluke in the execution...

@totten
Copy link
Member

totten commented Nov 18, 2019

jenkins, test this please

@mfb mfb force-pushed the string-type-declaration branch 3 times, most recently from da6f2ab to b9cb949 Compare November 19, 2019 04:10
@mfb mfb force-pushed the string-type-declaration branch from b9cb949 to 8f76fa8 Compare November 19, 2019 04:13
@mfb
Copy link
Contributor Author

mfb commented Nov 19, 2019

Various tests assume civicrm_api() $entity param is nullable, so rather than rewriting the tests and/or internal API code, I decided to leave this assumption intact for now.

@eileenmcnaughton
Copy link
Contributor

We should probably rip out those tests now. However, I'm OK to merge this now it's passing

@eileenmcnaughton eileenmcnaughton merged commit e4399b1 into civicrm:master Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants