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

Better respect PHP native array key handling for assertArrayIs*ToArrayOnlyConsideringListOfKeys() #5716

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Feb 29, 2024

Related to #5600

As things are, arrays in PHP can have either integer or string keys. Depending on the key input, PHP does some type juggling magic though, like auto-converting purely integer string keys to integers and flooring floating point keys to integers.

While experienced devs will know this pitfall, less experienced devs (who also write tests) may not be as aware and may provide the keys in $keysToBeConsidered the same way as the original array was defined, not realizing that the type of some of the keys will have auto-magically been changed by PHP.

The code in the new assertArrayIs*ToArrayOnlyConsideringListOfKeys() assertions, with its use of strict in_array() did not respect the key juggling PHP does, while the code for the assertArrayIs*ToArrayIgnoringListOfKeys assertions did (as unset() - and isset() for that matter - will do the same type juggling for the array keys).

This commit adjusts the code for the assertArrayIs*ToArrayOnlyConsideringListOfKeys() assertions to handle arrays keys passed in $keysToBeConsidered consistently in the same way PHP itself would do.

Includes tests.
Includes tests for the same for the assertArrayIs*ToArrayIgnoringListOfKeys assertions which were not affected by this bug.

…spect PHP native array key handling

Related to sebastianbergmann/phpunit 5600

As things are, arrays in PHP can have either integer or string keys.
Depending on the key input, PHP does some type juggling magic though, like auto-converting purely integer string keys to integers and flooring floating point keys to integers.

While experienced devs will know this pitfall, less experienced devs (who also write tests) may not be as aware and may provide the keys in `$keysToBeConsidered` the same way as the original array was defined, not realizing that the type of some of the keys will have auto-magically been changed by PHP.

The code in the new `assertArrayIs*ToArrayOnlyConsideringListOfKeys()` assertions, with its use of strict `in_array()` [did not respect the key juggling PHP does](https://3v4l.org/FdReu), while [the code for the `assertArrayIs*ToArrayIgnoringListOfKeys` assertions did](https://3v4l.org/AfHoc) (as `unset()` - and `isset()` for that matter - will do the same type juggling for the array keys).

This commit adjusts the code for the `assertArrayIs*ToArrayOnlyConsideringListOfKeys()` assertions to handle arrays keys passed in `$keysToBeConsidered` consistently in the same way PHP itself would do.

Includes tests.
Includes tests for the same for the `assertArrayIs*ToArrayIgnoringListOfKeys` assertions which were not affected by this bug.
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.96%. Comparing base (8bcc515) to head (754f2ec).

Additional details and impacted files
@@            Coverage Diff            @@
##               11.0    #5716   +/-   ##
=========================================
  Coverage     89.96%   89.96%           
  Complexity     6465     6465           
=========================================
  Files           683      683           
  Lines         19596    19600    +4     
=========================================
+ Hits          17630    17634    +4     
  Misses         1966     1966           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sebastianbergmann sebastianbergmann merged commit 7ec4599 into sebastianbergmann:11.0 Feb 29, 2024
25 checks passed
@sebastianbergmann sebastianbergmann added type/bug Something is broken feature/assertion Issues related to assertions and expectations labels Feb 29, 2024
@sebastianbergmann sebastianbergmann changed the title assertArrayIs*ToArrayOnlyConsideringListOfKeys(): bug fix - better respect PHP native array key handling Better respect PHP native array key handling for assertArrayIs*ToArrayOnlyConsideringListOfKeys() Feb 29, 2024
@jrfnl jrfnl deleted the feature/assertonlyconsidering-bugfix branch February 29, 2024 21:59
@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 29, 2024

Thanks for accepting this fix @sebastianbergmann !

Just in case anyone is wondering why this could be an issue, here's an example usecase where this could come into play with these assertions:

Think an integration test where the code under test inserts some data into a database and the tests wants to verify that the data was correctly added.

In the test:

  • The code under test would be run and returns an array of integer strings representing the inserted record ids.
  • Next a DB query would be run to retrieve the full records in the database table as an array indexed by the record ids.
  • Then the results of second query would be compared with the expected records for only the inserted record ids using the assertArrayIs*ToArrayOnlyConsideringListOfKeys() assertions using the return value of the code under test as $keysToBeConsidered.

By default (without an abstraction layer), most databases will return text strings, even for ids, so without this fix that comparison would have failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/assertion Issues related to assertions and expectations type/bug Something is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants