Skip to content

Conversation

@SanderMuller
Copy link
Contributor

Description:

This PR ensures that the blank helper does not attempt to serialize Eloquent models when checking if they are blank. The issue was encountered when using ->loadMissing() on a model, where calling the blank helper unexpectedly triggered jsonSerialize, leading to unintended behavior and overhead.

Changes:

  • Updated the blank helper to return false for instances of Illuminate\Database\Eloquent\Model.
  • Added a test to verify that models are not serialized when passed to the blank helper.

Benefits:

  • Prevents unexpected model serialization when using the blank helper, particularly in cases like ->loadMissing().
  • Avoids runtime exceptions or side effects caused by serialization during blank checks.
  • Improves developer experience by aligning the helper's behavior with expectations.

Impact:

This change is non-breaking and does not alter the behavior of the blank helper for non-model values. It ensures consistent and safe handling of Eloquent models.

@taylorotwell taylorotwell merged commit d288758 into laravel:11.x Dec 16, 2024
38 checks passed
@BernhardK91
Copy link

Today I stumbled over that bug that caused real problems when data for a model was fetched from an external API using an Accessor. That caused plenty of API calls when checking the variable (a model) was "blank" for thousands of objects, for example using the transform() helper function.

When I wanted to create a PR I saw it was already fixed. So thank you very much, @SanderMuller 🙏🏻

@SanderMuller
Copy link
Contributor Author

Today I stumbled over that bug that caused real problems when data for a model was fetched from an external API using an Accessor. That caused plenty of API calls when checking the variable (a model) was "blank" for thousands of objects, for example using the transform() helper function.

When I wanted to create a PR I saw it was already fixed. So thank you very much, @SanderMuller 🙏🏻

I'm glad I could help, thanks for the message!

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