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

[11.x] Stringable is also an interface of symfony #51309

Merged
merged 6 commits into from
May 6, 2024

Conversation

lava83
Copy link
Contributor

@lava83 lava83 commented May 6, 2024

Stringable is also an interface of symfony and is implemented on models. So if we check a model if a model is filled it except an error: Call to undefined method App\Model::trim(). So we use now explicit Illuminate\Support\Stringable

stefan.riedel added 4 commits May 5, 2024 17:02
…models. So if we check a model if a model is filled it except an error: Call to undefined method App\\Model::trim(). So we use now explicit Illuminate\Support\Stringable
@Jacobs63
Copy link
Contributor

Jacobs63 commented May 6, 2024

You've added an unused Illuminate\Support\Stringable import.. what exactly does this solve?

The interface is not used in the helpers file, it is only referenced via PHPDoc, so this only adds an unused duplicate of that.

@lava83
Copy link
Contributor Author

lava83 commented May 6, 2024

You've added an unused Illuminate\Support\Stringable import.. what exactly does this solve?

The interface is not used in the helpers file, it is only referenced via PHPDoc, so this only adds an unused duplicate of that.

No: src/Illuminate/Support/Stringable.php - used the global interface as BaseStringable

image

And if I check in the blank function the value is an instance of (Base)Stringable, filled($model) dont work because Model didnt have a methode trim.

@Jacobs63
Copy link
Contributor

Jacobs63 commented May 6, 2024

You've added an unused Illuminate\Support\Stringable import.. what exactly does this solve?
The interface is not used in the helpers file, it is only referenced via PHPDoc, so this only adds an unused duplicate of that.

No: src/Illuminate/Support/Stringable.php - used the global interface as BaseStringable

image

And if I check in the blank function the value is an instance of (Base)Stringable, filled($model) dont work because Model didnt have a methode trim.

Ah.. so this fixes your previous PR #51300

That's the difference, I see, sounds good!

@Jacobs63
Copy link
Contributor

Jacobs63 commented May 6, 2024

I believe updating the blank helper with this:

        if ($value instanceof Stringable) {
            return trim((string) $value) === '';
        }

Instead of this:

        if ($value instanceof \Illuminate\Support\Stringable) {
            return $value->trim()->toString() === '';
        }

Is both faster and solves more use-cases - @taylorotwell thoughts?

@lava83
Copy link
Contributor Author

lava83 commented May 6, 2024

I believe updating the blank helper with this:

        if ($value instanceof Stringable) {
            return trim((string) $value) === '';
        }

Instead of this:

        if ($value instanceof \Illuminate\Support\Stringable) {
            return $value->trim()->toString() === '';
        }

Is both faster and solves more use-cases - @taylorotwell thoughts?

But you're not going to hit strings alà ' '. Thats it why I've added $value->trim()

@driesvints driesvints changed the title Stringable is also an interface of symfony [11.x] Stringable is also an interface of symfony May 6, 2024
@lava83
Copy link
Contributor Author

lava83 commented May 6, 2024

I believe updating the blank helper with this:

        if ($value instanceof Stringable) {
            return trim((string) $value) === '';
        }

Instead of this:

        if ($value instanceof \Illuminate\Support\Stringable) {
            return $value->trim()->toString() === '';
        }

Is both faster and solves more use-cases - @taylorotwell thoughts?

Okay I've seen, native trim can trim this:

$value = '    '; //more than one spaces

trim($value) === ''; //true

I will change it to native php function. Thx @Jacobs63

@lava83 lava83 marked this pull request as draft May 6, 2024 08:39
@lava83 lava83 marked this pull request as ready for review May 6, 2024 08:51
@taylorotwell taylorotwell merged commit c9d2ba0 into laravel:11.x May 6, 2024
28 checks passed
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.

3 participants