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

Patch for #40197 #3

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Patch for #40197 #3

wants to merge 8 commits into from

Conversation

carlomanf
Copy link
Owner

Trac ticket: https://core.trac.wordpress.org/ticket/40197


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

carlomanf pushed a commit that referenced this pull request Jan 17, 2022
…itTestCase_Base::assertSameIgnoreEOL()`.

Basically, the whole `assertSameIgnoreEOL()` assertion was fundamentally flawed. The assertion contends that it checks that the expected and actual values are of the same type and value, but the reality was very different.

* The function uses `map_deep()` to potentially handle all sorts of inputs.
* `map_deep()` handles arrays and objects with special casing, but will call the callback on everything else without further distinction.
* The callback used passes the expected/actual value on to the `str_replace()` function to remove potential new line differences.
* And the `str_replace()` function will - with a non-array input for the `$subject` - always return a string.
* The output of these calls to `map_deep()` will therefore have "normalized" _all properties_ in objects, _all values_ in arrays and _all non-object, non-array values_ to strings.
* And a call to `assertSame()` will therefore NEVER do a proper type check as the type of all input has already, unintentionally, been "normalized" to string.

Aside from this clear flaw in the design of the assertion, PHP 8.1 now exposes a further issue as a `null` value for an object property, an array value or a plain value, will now yield a ` str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated` notice.

To fix both these issues, the fix in this PR ensures that the call to `str_replace()` will now only be made if the input is a text string.
All other values passed to the callback are left in their original type.

This ensures that a proper value AND type comparison can be done as well as prevents the PHP 8.1 deprecation notices.

Ref:
* https://developer.wordpress.org/reference/functions/map_deep/
* https://www.php.net/manual/en/function.str-replace.php

This commit:
- Fixes type-casting of non-string values to `string` (the flawed part of this assertion) by invoking `str_replace()` when the value is of string type.
- Fixes the PHP 8.1 `str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated` deprecation notice.
- Micro-optimization: skips `map_deep()` when actual and/or expected are `null` (no need to process).
- Adjusts the method documentation for both this method and the `assertEqualsIgnoreEOL()` alias method to document that the `$expected` and `$actual` parameters can be of any type.

Follow-up to [48937], [51135], [51478].

Props jrf, hellofromTonya.
See #53363, #53635.

git-svn-id: https://develop.svn.wordpress.org/trunk@51831 602fd350-edb4-49c9-b593-d223f7449a82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant