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

[10.x] Deprecate Stringable test and whenTest methods #46345

Closed
wants to merge 1 commit into from
Closed

[10.x] Deprecate Stringable test and whenTest methods #46345

wants to merge 1 commit into from

Conversation

localusercamp
Copy link
Contributor

What does this PR propose

  • Add whenMatch (or whenIsMatch, i don't know what is better 👀) method as alternative for whenTest
  • Move Illuminate\Support\Stringable test and whenTest methods to deprecated status

Why it do so

First reason

Recently isMatch method was added to Illuminate\Support\Str and Illuminate\Support\Stringable. It allows to check is string matching given pattern(s). There is also test method in Illuminate\Support\Stringable which do exactly the following:

str(string)->match(pattern)->isNotEmpty();

In prevoius PR, I explained why this is not fully correct way to check if string contains pattern.

From #46303:

match() in the other hand supports complex pattern matching but returns string that has been matched. It forces us to additionally check that result of the method call is not containing an empty string, which is not the same thing as check if a string contains pattern.

// I.e. Does 'foobar' contain pattern /f.*o.*/ ?
str('foobar')->match('/f.*o.*/')->isNotEmpty(); // true

// This is why match() followed by isNotEmpty() is not the same thing as checking string for pattern match

// I.e. Does 'foobar' contain pattern /^f.*r(.*)/ ?
str('foobar')->match('/^f.*r(.*)/')->isNotEmpty(); // false
// But preg_match for this pair returns
preg_match('/^f.*r(.*)/', 'foobar') === 1; // true

Second reason

Illuminate\Support\Stringable now has 2 methods for checking pattern matching which is ambigous in my opinion.

Third reason

In my opinion test is not the best name for function that matches string against pattern.

P.S.

I will glad to discuss this.

@taylorotwell
Copy link
Member

I would rather just update test to use isMatch under the hood as it seems like it's kind of a bug that it doesn't work that way right now based on how we have it documented.

@localusercamp
Copy link
Contributor Author

Ok. I will resubmit it then.

@localusercamp
Copy link
Contributor Author

@taylorotwell is that right to just replacing match()->isNotEmpty() with isMatch inside test method? That change might affect users code which already uses test, because isMatch could return true where match()->isNotEmpty() couldn't. Or I just misunderstood you.

I proposed this as deprecated exactly because we just can't replace it in test method in my opinion. What do you think?

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.

2 participants