Create old input wrapper to solve old() sessions problems.

[pxpm]

This is a second attempt to fix: #1816

First try here: #2848

After some discussion endup with this PR.

[pxpm]

@tabacitu

Why did we postponed this to 4.2, is a BC or it was because it changed all field files? cannot recall ❌ but I have a slim memory of it beeing about the second one.

EDIT: nvm, found it:

We should review this, or you still think the same ? If you changed your mind, please move back to 4.1.

I think we should do it.

This fix two current bugs with crud:

• old inputs beeing overriden by default when sent empty
• empty input beeing overriden by default on edit page

About the second issue, people (me included) had been working around it by creating two similar fields for create/update form and don't use the default => 'smt' in the update form field.

I'v just fixed the merge conflicts and this branch is now updated with master.

Best,
Pedro

[tabacitu]

Hey @pxpm - I'm still not comfortable with making changes in ALL fields in a non-breaking update. I have a feeling this might change behaviour for some people, that use the fields in an unexpected way. So it'd be better to do this in 4.2.

I'd rather re-prioritize projects and launch 4.2 sooner, than launch this in 4.1. A LOT of people are using 4.1, and we really don't want a flood of "this broke my app" messages, would be horrible for both us and the devs.

Let me know if you think I'm wrong and this is 100% non-breaking.

Cheers!

[promatik]

@pxpm can't we use the new function here?

[pxpm]

Hum .. how to say it .. No! 🙃

We can't @promatik , actually there is no sense at all to have value=smt in that input as it is totally impossible to set a file type input value programatically.

There are only 2 ways of dealing with this:

• Validate the form with ajax and only submit when you are sure form is not going to fail, so ... yeah.
• Before validation check if file exists in request, save as temporary and setup a mechanism to use that file if user didn't change it, otherwise do the cleanup of unused files.

Either way .. I just remove this value= from here, and let's hope for dropzone 🤞 Or forms validating with ajax before submiting. (that would be neat!)

[tabacitu]

I have no comments regarding this. There is nothing for us to do from my point of view, that's how upload inputs work, they can't prepopulate with the old value.

[tabacitu]

Pedro, I see the value is removed entirely here. Not even default registers any more. Was this intentional? Doesn't it have any downsides? Granted, I don't know who would set a default on this field... and why... and how it would work.

But I'm just wondering, since I see the upload field does have it - was this a mistake?

[pxpm]

No need to add anything under value= in file inputs. Don't have any effect..

[tabacitu]

I see there are a few fields that are NOT using old_input_value() now.
I'm not saying they should... I'm asking... why aren't they - is this intentional or a mistake?

• browse_multiple
• checklist_dependency
• enum
• password - it never pre-populates the value so I understand why this one doesn't;
• select2_from_array
• select2_multiple
• select_from_array
• select_multiple
• upload_multiple

Do they still have "the bug"? That's what matters after all. If they don't... we're good, no need to refactor to use old_input_value(). If they do have the bug, we should fix that, and the cleanest would probably be using old_input_value().

[pxpm]

@tabacitu I am taking care of this ... and I think I found why explicitly pass the fallback (what field should show when empty) to the old_input_value function. It's what should be returned whe input is present in the old() but it's null.

If we chain value ?? default ?? fallback as the "fallback", if we are editing an entry we will never reach the default or fallback because the value would already exists. And when you submit the input empty it will show the value instead of the fallback as it should.

A written example:

• text field in monster crud.
• create a monster and then go edit it.
• clear the text field so the validation fails.
• Now you will get whatever is present in the old() - FINE, IT WILL BE EMPTY
• But it will fail for example in fields that require the "fallback" to be [] or that don't parse the "null" well.

We can:

• Use a third parameter as a solo fallback. old_input_value(field_name, valueOrDefault, fallback)

function old_input_value($input_name, $valueOrDefault, $fallback)
    {
        $input_name = square_brackets_to_dots($input_name);
        $old_inputs = session()->getOldInput();

        // if the input name is present in the old inputs we need to return earlier and not in a coalescing chain
        // otherwise `null` aka empty will not pass the condition and the field value would be returned.
        if (\Arr::has($old_inputs, $input_name)) {
            return \Arr::get($old_inputs, $input_name) ?? $fallback; //so if it's null here, we want to return the field "empty state"
        }

        return $valueOrDefault;
    }

Some parts of my body were telling me something isn't rigth when I was refactoring ... We had talked about this issue sometimes, and as I recall we started doing as you suggested the refactor (how it is currently implemented) and then ended-up sending more stuff to the function.

Let me know what you think before I refactor this.

Pedro

[tabacitu]

Maybe we do...

has_old_input('title') ? get_old_input('title', $fallback) : $value ?? $default ?? $fallback;

old_input_fallback_or_null('title', '') ?? $value ?? $default ?? '';
old_input_value('title', '[]') ?? $value ?? $default ?? '[]';
old_empty_or_fallback('title', [], $value ?? $default);

[tabacitu]

@pxpm let's do it with old_empty_or_fallback(). The name isn't very clear... but it's clear ENOUGH. And if to the dev it isn't clear enough, they can click on it to go to definition, where we'll provide a comprehensive explanation. So:

/**
 * This method is an alternative to Laravel's old() helper, which mistakenly
 * returns NULL it two cases:
 * - if there is an old value, and it was empty or null
 * - if there is no old value
 *
 * In contrast, this method will return:
 * - the old value, if there is one for that input;
 * - the second parameter, if there is no old value for that input;
 * - the third parameter, if there is no old input at all;
 */
function old_empty_or_fallback($input_name, $empty, $fallback)
    {
        $input_name = square_brackets_to_dots($input_name);
        $old_inputs = session()->getOldInput();

        // if the input name is present in the old inputs we need to return earlier and not in a coalescing chain
        // otherwise `null` aka empty will not pass the condition and the field value would be returned.
        if (\Arr::has($old_inputs, $input_name)) {
            return \Arr::get($old_inputs, $input_name) ?? $empty; // so if it's null here, we want to return the field "empty state"
        }

        return $fallback;
    }

I think this is the best we can do for now. Strikes an "ok" balance between simplicity, legibility and smaller breaking change. Don't you think? 💪

[tabacitu]

Pedro, I noticed you refactored and encountered a big problem: we can't call old_empty_or_fallback('title', [], $value ?? $default); because there IS NO DEFAULT most of the time so it'll error. So we end up calling old_empty_or_fallback('title', [], $value ?? $default ?? ''); anyway. That means we end up typing the empty string twice anyway...

If that's true, change of plans. Let's do this instead:

old_empty_or_null('title', '') ?? $value ?? $default ?? '';

It's ugly... but it works (at least from my tests). And it's a one-liner. Here's the function I used, with a very good description (I think):

if (! function_exists('old_empty_or_null')) {
    /**
     * This method is an alternative to Laravel's old() helper, which mistakenly
     * returns NULL it two cases:
     * - if there is an old value, and it was empty or null
     * - if there is no old value
     * (this is because of the ConvertsEmptyStringsToNull middleware)
     *
     * In contrast, this method will return:
     * - the old value, if there actually is an old value for that key;
     * - the second parameter, if there is no old value for that key, but it was empty string or null;
     * - null, if there is no old value at all for that key;
     *
     * @param  string $key
     * @param  array|string $empty_value
     * @return mixed
     */
    function old_empty_or_null($key, $empty_value = '')
    {
        $key = square_brackets_to_dots($key);
        $old_inputs = session()->getOldInput();

        // if the input name is present in the old inputs we need to return earlier and not in a coalescing chain
        // otherwise `null` aka empty will not pass the condition and the field value would be returned.
        if (\Arr::has($old_inputs, $key)) {
            return \Arr::get($old_inputs, $key) ?? $empty_value;
        }

        return null;
    }
}

This will solve all our problems, right?

[pxpm]

Indeed @tabacitu

I stopped yesterday but it was late and I was tired so I let it boil down during the nigth to come with fresh eye today.

Totally agree with your reasoning.

Back to you here in a few 👍

[tabacitu]

Answered!!! We have a way forward 🎉🎉🎉

[tabacitu]

@pxpm noticed you're having problems with what we've decided. See my thoughts in #3054 (comment) I think we've finally got it - old_empty_or_null()! By eliminating all other options, but hey... at least we've got it 😅

[pxpm]

@tabacitu I think it's finished here.

• upload and upload multiple don't need access to old() because file inputs can't be given value programatically.
• custom_html and view are also out of scope here.

I don't know how many times in the last days I've been testing this fields, I didn't found nothing broken, but it could be overlooking something by now.

Let me know,
Pedro

[tabacitu]

Awesome! Gave it a whirl too, tested all fields and it looks like everything's fine.
We'll make sure to test this again before we launch 4.2, though.

Let's do this!!! 💪🎉🎉🎉

[tabacitu]

Thank you so much for bearing with me, btw. I know we've spent waaay too much time on this, but it was a tricky one to get right.

[howest-ward]

As far I can see the bug with the default being used when updating a record is still there.
When I edit a record and I get null from the database, the default is used. This should never happen!

[pxpm]

@howest-ward thanks for the feedback.

I don't think that's related with this issue, this was specifically related with the old() helper function, and getting the errors/values from the session.

From my understanding of the issue you mentioned, you are just editing an entry, and if that entry has a default, but it's null on database it's populating with the default value ?

Anyway, this is a ~4 years old issue, please open a new issue with clear reproduction steps and I can have a look at it. 🙏

Cheers