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

Declare strict types in CiviUnitTestCase #26002

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Declare strict types in CiviUnitTestCase

Before

Strict types not declared in unit tests

After

Strict types declared on main base class

Technical Details

My reading of
https://www.phptutorial.net/php-tutorial/php-strict_types/

is that

  1. declaring strict type declarations in tests might help us pick up bugs we would otherwise miss and
  2. I believe that we would have to declare it in EVERY test file, rather than it inheritting to child classes

Comments

Let's see what this throws up...

@civibot
Copy link

civibot bot commented Apr 4, 2023

(Standard links)

@civibot civibot bot added the master label Apr 4, 2023
@eileenmcnaughton
Copy link
Contributor Author

I'll figure out the style once I see how the rest plays out...

@eileenmcnaughton
Copy link
Contributor Author

test this please

1 similar comment
@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

test this please

Declare strict types in test
@eileenmcnaughton
Copy link
Contributor Author

test this please

@colemanw
Copy link
Member

@eileenmcnaughton I'm really surprised that passed. Are we sure it actually worked?
Does strict_types prevent you from passing a numeric string to a function that requires a number? Like

function foo(int $id) {}`
foo('123');

Because we do that a lot.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw the first time I tried this it failed on passing NULL to regex - but note I think it only works on functions called directly from the class in question & I did a pass a while back on that

@eileenmcnaughton eileenmcnaughton merged commit c3154fb into civicrm:master Oct 23, 2023
@eileenmcnaughton
Copy link
Contributor Author

@colemanw ALSO I think type hinting on numbers will co-erce numeric to int (at least in some php versions)

@eileenmcnaughton eileenmcnaughton deleted the declare_strict branch October 23, 2023 03:30
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