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

[9.x] Stringable exactly not match with other Stringable variable #41846

Merged
merged 5 commits into from
Apr 6, 2022
Merged

[9.x] Stringable exactly not match with other Stringable variable #41846

merged 5 commits into from
Apr 6, 2022

Conversation

medeiroz
Copy link
Contributor

@medeiroz medeiroz commented Apr 6, 2022

  • Laravel Version: 9.5.1
  • PHP Version: 8.1
  • Database Driver & Version: N/A

Description:

When need compare two Stringable variables with exactly method. Then false is returned

Steps To Reproduce:

<?php

$first = \Str::of('name');
$second = \Str::of('name');
$first->exactly($second);
//  => false

// true is expected but false is returned

image

Solution

  • Cast argument to string when argument is Stringable

@SjorsO
Copy link
Contributor

SjorsO commented Apr 6, 2022

Doesn't make a lot of sense to have a method called exactly that does a loose comparison.

Might be a better idea to do this instead:

if ($value instanceof Stringable) {
    $value = $value->toString();
}

return $this->value === $value;

@driesvints driesvints closed this Apr 6, 2022
@driesvints driesvints reopened this Apr 6, 2022
@imanghafoori1
Copy link
Contributor

if (is_object($value) && method_exists('__toString', $value)) {
    $value = (string) $value;
}

So that there is no loose comparison will happen.

@dennisprudlo
Copy link
Contributor

dennisprudlo commented Apr 6, 2022

@imanghafoori1 I'm not sure every object that has a __toString() should be a valid parameter.

$model = new \App\Models\SomeModel;
str('[]')->exactly($model); // true

The type in the docblock says only string can be passed, so maybe just add Stringable (the class not the global interface) to the docblock and check like @SjorsO did.

Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👎 You are incorrectly calling this function if you do not pass a string.

@GrahamCampbell
Copy link
Member

If you want this function to support stringable objects, then we would have to widen its param type.

@GrahamCampbell GrahamCampbell changed the title Stringable exactly not match with other Stringable variable [9.x] Stringable exactly not match with other Stringable variable Apr 6, 2022
@driesvints
Copy link
Member

@GrahamCampbell that's what this PR tries to do 😅 I also misjudged this PR at first for wrong usage but I can see the use case here.

@pedroaff
Copy link

pedroaff commented Apr 6, 2022

The two itens has the exact values, so it should not return false.
It's makes sense when we need to compare two itens w/ the same value.

@taylorotwell taylorotwell merged commit 6ed23a5 into laravel:9.x Apr 6, 2022
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.

None yet

8 participants