Open MikeyBeLike opened 7 months ago
hi @MikeyBeLike
Since this change could break some compatibility, we will add it to our to-do list for v7. However, we see it as feasible to apply this modification for that version. Thank you for your contribution.
Cheers.
hi @MikeyBeLike
Since this change could break some compatibility, we will add it to our to-do list for v7. However, we see it as feasible to apply this modification for that version. Thank you for your contribution.
Cheers.
would it make more sense to update the files that are using nullish coalescing (??) instead? as the current implementation is broken anyway:
e.g instead of
$id = $this->crud->getCurrentEntryId() ?? $id;
do this:
$id = $this->crud->getCurrentEntryId() === FALSE ?: $id;
Hey @MikeyBeLike it would still a BC, because previously if the entry was not found with getCurrentEntry()
it wouldn't use the $id
, it would be false
.
If now we return the $id
, people with checks in place for if($entry)
would have a BC in their packages.
v7 is around the corner, we will def. do this. Either make it return properly null (and keep the calls as they are with the ??
), or keep it false but change the calls in our operations, we need to think of the benefits/implications of the change in both sides.
Thanks for your patience 🙏
WHY
BEFORE - What was wrong? What was happening before this PR?
getCurrentEntryId()
returns false, but ideally it should be returning null if unable to retrieve a value.There are also several places in the codebase that use this line to get the crud ID:
A few files:
src/app/Http/Controllers/Operations/DeleteOperation.php
src/app/Http/Controllers/Operations/ListOperation.php
src/app/Http/Controllers/Operations/ShowOperation.php
src/app/Http/Controllers/Operations/UpdateOperation.php
This will never allow you to fallback to
$id
using null coalescing since it returns false and doesn't return null.AFTER - What is happening after this PR?
change the return type from false to null, so that we can make use of fallbacks via null coalescing
HOW
How did you achieve that, in technical terms?
update the return type from false -> null
Is it a breaking change?
potentially if people are strictly checking for
FALSE
but if looselynull
is falsy so should be fineHow can we test the before & after?
??
If the PR has changes in multiple repos please provide the command to checkout all branches, eg.: