Skip to content

Differentiate explicit null from default using __UNSET__ in whenLoaded method #55381

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

Conversation

HichemTab-tech
Copy link

Fixes #55374.

Description

Due to the PHP 8 named arguments feature, we encountered an ambiguity issue. Previously, when calling methods like whenLoaded, explicitly passing a null argument was indistinguishable from omitting the argument completely. This ambiguity could cause unexpected results, like unintentionally returning the entire related entity instead of the intended null value.

For example, before this fix, the following call:

'value' => $this->whenLoaded('entity', $this->resource->entity->comment),

would incorrectly return the whole 'entity' object if comment was null in the database (and that's because of the changes intriduced in #51342 because of null verification)

{
  "value": {
    // entity object returned instead of null
  }
}

To resolve this ambiguity clearly and effectively, we've introduced a sentinel default value ('__UNSET__') to reliably identify if an argument was explicitly provided as null or simply omitted.

Changes

  • Before:

    protected function whenLoaded($relationship, $value = null, $default = null)

    (Cannot distinguish an explicitly provided null from omitted parameter)

  • After:

    protected function whenLoaded($relationship, $value = '__UNSET__', $default = null)

    (Clearly distinguish an explicitly provided null from omitted parameter)

Impact of this Change

With this fix, explicitly passing null will behave correctly and predictably:

  • Calling whenLoaded('relation', null) explicitly returns null as intended.
  • Improves internal logic by clearly differentiating between omitted parameters and explicitly provided null.

Now, the example above returns the correct result:

{
  "value": null
}

Updated default parameter values in `whenLoaded`, `whenCounted`, and `whenAggregated` methods to use '__UNSET__' instead of null. This ensures clearer intent and resolves potential ambiguities when null is passed explicitly as a value.
@HichemTab-tech HichemTab-tech marked this pull request as ready for review April 12, 2025 17:01
@AndrewMast
Copy link
Contributor

Just as a note, a string like __UNSET__ hasn't been used before in the Laravel framework. Because of this, it will probably be denied. Usually, new stdClass is used to determine if a method is returning the default value (see Collection::contains).

@AndrewMast
Copy link
Contributor

AndrewMast commented Apr 12, 2025

Maybe an alternative to if ($value === null) could be if (func_num_args() === 1), so if someone passes in null for $value, it won't trigger it.

@HichemTab-tech
Copy link
Author

Maybe an alternative to if ($value === null) could be if (func_num_args() === 1), so if someone passes in null for $value, it won't trigger it.

Well unfortunately no, with named arguments of php 8 if we call the third argument without the second like this :

whenLoaded('relation', default: null)

The func_num_args still returns 3 :/

@HichemTab-tech
Copy link
Author

Just as a note, a string like __UNSET__ hasn't been used before in the Laravel framework. Because of this, it will probably be denied. Usually, new stdClass is used to determine if a method is returning the default value (see Collection::contains).

yes the problem here is we may have the model property cast to stdClass, so we would still fall in the case where we dont know if the stdClass is provided or unspecified by default

$this->whenLoaded('entity', $this->resource->entity->comment),
protected $casts = [
    'comment' => 'object',
];

So in this case the comment would be stdClass (I think).

@taylorotwell
Copy link
Member

Potential breaking change.

@HichemTab-tech
Copy link
Author

HichemTab-tech commented Apr 13, 2025

Potential breaking change.

Sorry @taylorotwell but i believe there is no action required by users at all in this case (no breaking change for users at least), this PR will just fix the problem caused by #51342 please check the PR mentioned and the related issue #55374

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.

After #51342 whenLoaded doesn't return null when needed
3 participants