[Core: Database] Add type declarations to function signatures #3878

johnsaigle · 2018-08-14T17:46:28Z

Brief summary of changes

Type declarations and return type declarations are added to all function signatures in the Database class. This follows a discussion about moving toward the universal use of PHP's typing features to prevent notices, warnings, and other nasty behaviour resulting from type coercion.

I tried not to alter any functionality by following the return types already listed in the PHP Docs for each function:

In some cases where the documentation conflicted with the actual return type, the documentation was amended and the functionality preserved.
Functions that were documented as returning void but had a return statement were changed to not return anything.
~~Default values were largely left as is even in cases where another might arguably be better (e.g. Strings set to null instead of the empty string)~~
- This has been changed based on feedback by @driusan .

Further work to be done

Some of the functions were not given return types (notably all pselect* functions) because they return mixed types. We need to refactor these functions to behave consistently.

This resolves issue...

Nothing concrete but many potential issues arising from PHP's type coercion.

Related PRs:
A check means it's merged

To test this change...

Important if you get an error about a return type like ...must be an instance of void, none returned, that means the version of PHP being used by your Apache is older than PHP 7.1. As of the last release we require >= 7.2 so be sure to correct this.

The Database class should work as before. If it doesn't, it has been used improperly and the calling classes should be modified to use it as documented.

driusan · 2018-08-16T14:04:38Z

(Added Needs Work because it's failing Travis, haven't looked into why.)

PapillonMcGill

LGTM

johnsaigle · 2018-08-20T14:31:51Z

@driusan I believe it's failing in the cases in LORIS where we have baked-in type coercion. It's likely that several smaller PRs normalizing the use of function calls and return values from this function will need to be merged before this can be merged.

Perhaps Blocked is more appropriate than Needs Work?

driusan · 2018-08-20T14:54:46Z

Sure, I changed the tag.

driusan · 2018-08-21T17:03:46Z

php/libraries/Database.class.inc

-        $trackChanges = true
+        ?string $username,
+        ?string $password,
+        ?string $host,


This one seems weird.. what does it mean to pass those as null?

Honestly not totally sure. It requires further research. However when these are set to just strings (null not allowed) Travis throws up a ton of errors during the execution of unit tests. Something to the effect of "MockDatabase::connect() requires a string parameter for param X but null given".

I don't know enough about our automated testing to know what to do about that, so I changed this function to allow for null. I'm very open to suggestions here.

@kongtiaowang Any thoughts on how to solve that?

@johnsaigle other than resolving this ^ issue and travis, i think everything else looks good. will test in a sec

apache php7.2 upgrade not working for me so can't test at the moment

IMO we should go ahead and merge this. I think the nullable strings thing is likely a quirk of PHPUnit. In any case if any of the params is null then the DB will error out anyway due to invalid credentials.

In an effort to take advantage of PHP's type system, we should add type declarations to our code, especially in the core classes. The PDO fetchAll returns the empty set when no data in the DB matches a given query and FALSE on failure. However we do not distinguish this in our codebase and so a FALSE value is allowed to bubble up through subsequent DB calls. This makes it so that the database class throws an exception in the failure case following discussion in #3854. This will allow us to remove Bool as one of the return types in the future. Related issues: #3854, #3878

driusan · 2018-08-27T17:53:20Z

php/libraries/Database.class.inc

+        string $database     = null,
+        string $username     = null,
+        string $password     = null,
+        string $host         = null,


This is going to fail, null isn't a sting.

I ran phan locally and it didn't seem to have an issue.... which is weird cause you're right.

I'll change this and the if statement below.

That's.. problematic? Maybe it just checks it at the call site and it's never called without them being explicitly passed?

Maybe? Not sure how the internals of phan work.

johnsaigle · 2018-08-29T18:32:09Z

This depends on #3909 to pass Travis. Will rebase this once that is merged.

driusan · 2018-08-29T18:43:07Z

Maybe this PR should also add declare(strict_types=1); to the file?

johnsaigle · 2018-08-29T18:47:11Z

Yeah I had that earlier but think I removed it because something was going wrong and I thought it was the cause. I'll add it back.

driusan · 2018-09-10T14:08:34Z

@johnsaigle #3909 is merged

zaliqarosli

first round changes

php/libraries/Database.class.inc

kongtiaowang · 2018-10-15T15:16:20Z

It will help you to sove the Travis issue.

zaliqarosli · 2018-10-15T16:17:56Z

php/libraries/Database.class.inc

-        $trackChanges = true
+        ?string $username,
+        ?string $password,
+        ?string $host,


@johnsaigle other than resolving this ^ issue and travis, i think everything else looks good. will test in a sec

… into 180814-FunctionSignaturesDatabase

addressed

resolved

johnsaigle added Category: Cleanup PR or issue introducing/requiring at least one clean-up operation [branch] major labels Aug 14, 2018

johnsaigle added this to the 21.0.0 milestone Aug 14, 2018

johnsaigle changed the title ~~[Core] Add type declarations to function signatures~~ [Core: Database] Add type declarations to function signatures Aug 14, 2018

This was referenced Aug 15, 2018

[Database] Change return type of pselectOne to null #3854

Closed

[Core: Database] Add exception on fetchAll failure case #3880

Merged

driusan added the State: Needs work PR awaiting additional work by the author to proceed label Aug 16, 2018

PapillonMcGill reviewed Aug 20, 2018

View reviewed changes

driusan added State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed and removed State: Needs work PR awaiting additional work by the author to proceed labels Aug 20, 2018

driusan reviewed Aug 21, 2018

View reviewed changes

driusan reviewed Aug 27, 2018

View reviewed changes

johnsaigle mentioned this pull request Aug 29, 2018

[Data Team Helper] Ensure test_name is always a string #3909

Merged

driusan removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Sep 10, 2018

zaliqarosli requested changes Sep 10, 2018

View reviewed changes

php/libraries/Database.class.inc Outdated Show resolved Hide resolved

php/libraries/Database.class.inc Show resolved Hide resolved

php/libraries/Database.class.inc Outdated Show resolved Hide resolved

php/libraries/Database.class.inc Outdated Show resolved Hide resolved

PapillonMcGill reviewed Sep 11, 2018

View reviewed changes

php/libraries/Database.class.inc Show resolved Hide resolved

johnsaigle mentioned this pull request Sep 11, 2018

[Core: Database] Add type check to HTMLEscapeArray #3947

Merged

johnsaigle added the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Oct 2, 2018

kongtiaowang removed the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Oct 15, 2018

zaliqarosli previously requested changes Oct 15, 2018

View reviewed changes

johnsaigle mentioned this pull request Oct 15, 2018

[Statistics] Change pselect calls to pass array()s, not null #4003

Merged

1 task

Fixed param that should be nullable

02a5417

Merge remote-tracking branch 'john/180814-FunctionSignaturesDatabase'…

7990545

… into 180814-FunctionSignaturesDatabase

PapillonMcGill approved these changes Nov 6, 2018

View reviewed changes

johnsaigle added the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Nov 19, 2018

johnsaigle mentioned this pull request Nov 19, 2018

[Core: LorisForm] Add type declarations to function signatures #4122

Closed

This was referenced Nov 19, 2018

[Core: NDB_Factory] Add type declarations to function signatures #4123

Merged

[Core: BVL_Feedback_Panel] Add type declarations to function signatures #4124

Merged

[Core: NDB_Caller] Add type declarations to function signatures #4125

Merged

driusan merged commit 426fe51 into aces:major Nov 19, 2018

This was referenced Nov 20, 2018

[Core: InstrumentStatus] Add type declarations to function signatures #4126

Merged

[Core: Config] Add type declarations to function signatures #4127

Merged

[Core: NDB Page and Children] Add return types to function signatures #4139

Closed

johnsaigle mentioned this pull request Nov 27, 2018

[Core: Utility] Deprecate Utility::toArray() #4151

Merged

2 tasks

johnsaigle removed the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Dec 12, 2018

This was referenced Jan 28, 2019

[Testing] Add PhanTypeVoidAssignment to static analysis #4276

Merged

Add return types to Database class "pselect*" functions #4284

Closed

[Core: Database] Add type declarations to function signatures #3878

[Core: Database] Add type declarations to function signatures #3878

Uh oh!

Conversation

johnsaigle commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Brief summary of changes

Further work to be done

This resolves issue...

To test this change...

Uh oh!

driusan commented Aug 16, 2018

Uh oh!

PapillonMcGill left a comment

Choose a reason for hiding this comment

Uh oh!

johnsaigle commented Aug 20, 2018

Uh oh!

driusan commented Aug 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnsaigle Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnsaigle commented Aug 29, 2018

Uh oh!

driusan commented Aug 29, 2018

Uh oh!

johnsaigle commented Aug 29, 2018

Uh oh!

driusan commented Sep 10, 2018

Uh oh!

zaliqarosli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kongtiaowang commented Oct 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

johnsaigle commented Aug 14, 2018 •

edited

Loading

johnsaigle Aug 22, 2018 •

edited

Loading