PHPUnit 11.5.0 | AssertContainsOnly trait: polyfill the Assert::assertContains[Not]Only*() methods #225
Conversation
…tContains[Not]Only*() methods
PHPUnit 11.5.0 introduced a range of new assertions. These assertions are specialized alternatives to the pre-existing `assert[Not]ContainsOnly()` methods, which are now soft deprecated and will be removed in PHPUnit 13.0.
This commit:
* Adds two traits with the same name.
One to polyfill the methods when not available in PHPUnit.
The other to allow for `use`-ing the trait in PHPUnit versions in which the methods are already natively available.
* Adds logic to the custom autoloader which will load the correct trait depending on the PHPUnit version used.
* Adds tests.
Most polyfilled methods can just fall-through to the pre-existing methods as a polyfill.
The exceptions to this rule are:
* `assertContains[Not]OnlyIterable()` which needs custom logic for PHPUnit < 7.1.0 in which the PHP native `iterable` type was not yet supported.
* `assertContains[Not]OnlyClosedResource()` which needs custom logic for PHP < 7.2 in which the PHP native `resource (closed)` type did not exist yet.
In this last case, the custom logic is used for the polyfill on all PHP versions as there is no functional check (in contrast to a version check) which can be used to determine whether the `resource (closed)` type is available.
All new methods are loosely tested.
Includes:
* Adding information on the new polyfill to the README.
* Adding the new polyfill to the existing `TestCases` classes.
Refs:
* sebastianbergmann/phpunit@a726e03
Fixes 214
| * | ||
| * @return void | ||
| */ | ||
| final public static function assertContainsNotOnlyInstancesOf( string $type, $haystack, string $message = '' ) { |
There was a problem hiding this comment.
@jrfnl Refresh my memory please. Should the new polyfills add the string type declaration in the signature?
I'm asking because a lot of the polyfills do not (some do). Thanks.
There was a problem hiding this comment.
Valid question and the answer is "yes".
The reasoning for this is as follows:
- General speaking the function signature for a polyfill should be the same as the signature in PHPUnit itself, providing the minimum PHP version of the PHPUnit Polyfill version allows for it.
In this case, that means, thestringtype for$typeand$messagecan be added, but theiterabletype for$haystackcannot be added yet (needs PHP 7.1+, while the Polyfills 3.x series has a PHP 7.0 minimum). - For those types which cannot be added on introduction of polyfill functions, the Polyfills package follows PHPUnit itself.
This means that missing types will be added to individual polyfills once the minimum PHPUnit version supported by the polyfill aligns with the PHPUnit version in which the types were added in PHPUnit itself (as at that point the minimum supported PHP version of the Polyfills package will also allow for adding those types).
PHPUnit 7.0 introduces scalar types and return types to most assertions, so once the minimum supported PHPUnit version of the Polyfills is 7.0, the function signature of pre-existing polyfills will be updated to mirror the changes made in PHPUnit 7.0.
Along the same lines, PHPUnit 8.0 addedstrict_typesto PHPUnit itself. Once the Polyfills have a minimum of PHPUnit 8, this will be applied to the polyfills too.
Does that help and provide a clear enough guideline to follow for this ?
There was a problem hiding this comment.
Aha, yes, makes sense. Thanks for sharing this information. I'll bookmark as a guideline.
README.md
Outdated
| | [`Assert::assertContainsOnlyClosedResource()`] * | [`Assert::assertContainsNotOnlyClosedResource()`] * | | ||
| | [`Assert::assertContainsOnlyScalar()`] | [`Assert::assertContainsNotOnlyScalar()`] | | ||
| | [`Assert::assertContainsOnlyString()`] | [`Assert::assertContainsNotOnlyString()`] | | ||
| | [`Assert::assertContainsNotOnlyInstancesOf()`] | | |
There was a problem hiding this comment.
Though there's only the "Not" version, should this assert be listed in the "Not" (inverse) column for consistency?
There was a problem hiding this comment.
Fair point.
Makes me think I should probably reformat the table for the AssertIsType polyfills too.
| * | ||
| * @return void | ||
| */ | ||
| final public static function assertContainsNotOnlyInstancesOf( string $type, $haystack, string $message = '' ) { |
There was a problem hiding this comment.
Why list assertContainsNotOnlyInstancesOf() first? In the Framework/Assert.php file, it's listed after assertContainsNotOnlyString(). Wondering if placement should match PHPUnit for comparison? What do you think @jrfnl?
There was a problem hiding this comment.
I haven't used the order in the Framework/Assert.php file as a guideline anywhere, though at times the assertion order may align, in which case it's more by accident than by design.
In this case, there are two - very arbitrary - reasons for the order:
- It was listed first in the changelog of PHPUnit 11.5.0 ;-)
- All the other assertions are basically sets of two assertions, while this is a single new assertion. Using some form of alphabetic ordering would bury the assertion in the middle of those sets making this "odd one out" hard to find. Which leaves having it either as the first or the last method in the class and I, arbitrarily, choose to place it as the first.
There was a problem hiding this comment.
Yeah, I too noticed it was listed first in the changelog. That's a good point.
I followed the PHPUnit file during the review, which caught my attention. Point 2 makes sense.
Thanks for sharing your reasons.
PHPUnit 11.5.0 introduced a range of new assertions. These assertions are specialized alternatives to the pre-existing
assert[Not]ContainsOnly()methods, which are now soft deprecated and will be removed in PHPUnit 13.0.This commit:
One to polyfill the methods when not available in PHPUnit.
The other to allow for
use-ing the trait in PHPUnit versions in which the methods are already natively available.Most polyfilled methods can just fall-through to the pre-existing methods as a polyfill.
The exceptions to this rule are:
assertContains[Not]OnlyIterable()which needs custom logic for PHPUnit < 7.1.0 in which the PHP nativeiterabletype was not yet supported.assertContains[Not]OnlyClosedResource()which needs custom logic for PHP < 7.2 in which the PHP nativeresource (closed)type did not exist yet.In this last case, the custom logic is used for the polyfill on all PHP versions as there is no functional check (in contrast to a version check) which can be used to determine whether the
resource (closed)type is available.All new methods are loosely tested.
Includes:
TestCasesclasses.Refs:
Fixes #214