[Bug] Repeatable subfields not getting the correct old() value if there's a field with the same name in the main form #4029

pxpm · 2021-12-22T17:30:44Z

WHY

BEFORE - What was wrong? What was happening before this PR?

As reported in #4027 there seemd to be something wrong with repeatable and old values.

While testing I found out the culprit of this was the fact that old($field_name) returns true in case of M-M relationships because the repeatable name itself, it's the name of the pivot field.

In M-M relationships, or in case your repeatable name have the same name as some field inside the repeatable when that field gets it's old() value from session, it will get the FULL repeatable value and not that specific row value.

AFTER - What is happening after this PR?

After the PR if the returned from old is not what we expect (a single field value, but the whole repeatable value) we will parse it accordingly.

HOW

How did you achieve that, in technical terms?

Introduced row_key that are passed through repeatable_relation reaching the field. When setting up field value, we check if that row_key is set, if it is, we parse the value accordingly.

Is it a breaking change or non-breaking change?

4.2

How can we test the before & after?

Add a belongstomany relation, create 2 or 3 entries. Update the entry making the validation fail and see that ALL the pivots have the same entry selected.

BEFORE UPDATE:

ON UPDATE WITH VALIDATION ERRORS:

@tabacitu 2 things that I think that are worth talking about:

this is fixed only for relationship selects, but could happen with any field. This should be fixed in Create old input wrapper to solve old() sessions problems. #3054 that when getting the old value should be aware of the row_key parameter for repeatable and parse the value there. This would make the fix work for ANY field used inside the repeatable, because the principle it's the same.
In the images you can see that the field shows red validation, but it shows the "field previous value" not what I've sent to be validate (empty string). This is also fixed in Create old input wrapper to solve old() sessions problems. #3054

Let me know what you think,
Pedro

scrutinizer-notifier · 2021-12-22T17:42:29Z

The inspection completed: 544 Issues, 80 Patches

PitchRE · 2021-12-22T18:22:33Z

Hello. Thank you for quick response.
I have pulled this PR as "backpack/crud": "4.2.x-dev#439731408478c265fe2bf423a1960b3e6ee24869 as 4.1.99",.
Verified that my vendor directory has updated files.
Unfortunately, in my case exception did not dissapear.

Flareapp exception

OriginCrudController.php
OriginRequest.php

My characters relation definition.

    public function characters(): BelongsToMany
    {

        return $this->belongsToMany(Character::class);
    }

pxpm · 2021-12-22T18:25:45Z

Hello. Thank you for quick response. I have pulled this PR as "backpack/crud": "4.2.x-dev#439731408478c265fe2bf423a1960b3e6ee24869 as 4.1.99",. Verified that my vendor directory has updated files. Unfortunately, in my case exception did not dissapear.

Flareapp exception

OriginCrudController.php OriginRequest.php

My characters relation definition.
    public function characters(): BelongsToMany
    {

        return $this->belongsToMany(Character::class);
    }

You need to define: ->withPivot('pivot_fieldds') in your relation to work with pivot fields.

PitchRE · 2021-12-22T18:30:25Z

I just created another fresh Crud controller so I could easily share it here and used different model just to make sure. That's why withPivot() was missing. (forgot to add it)

    public function characters(): BelongsToMany
    {

        return $this->belongsToMany(Character::class)->withPivot('as');
    }

I can confirm that it doesn't work either way.

PitchRE · 2021-12-22T22:13:34Z

src/resources/views/crud/fields/relationship/fetch_or_create.blade.php

+                        $current_value = array($current_value[$field['row_key']][$field['name']]);
+                    }
+                }
+
                $current_value = $connected_entity


@pxpm
Keep in mind that I have no idea about backpack source code so I might be completly wrong.
You expect here $current_value to be an array of ids.
I logged it and that's what I got.
Sometimes its just array of ids, sometimes its array of arrays.
Why? I don't know.

[2021-12-22 22:07:41] local.DEBUG: array ( 0 => '1', ) [2021-12-22 22:07:41] local.DEBUG: array ( 0 => '2', ) [2021-12-22 22:07:41] local.DEBUG: array ( 0 => array ( 'characters' => '1', 'as' => 'noooo', ), 1 => array ( 'characters' => '2', 'as' => NULL, ), )

So obviously, ->whereIn($connected_entity_key_name, $current_value) will throw an error. Because $current_value is not array of ids but multidimensional array.
I made changes to relationship_select.blade.php in order to make it work.
Don't take this code too seriously.

case 'array': // check if field have `row_key` set, if it has we are in a repeatable // and should get the current row value if(isset($field['row_key'])) { if (isset($current_value[$field['row_key']]) && isset($current_value[$field['row_key']][$field['name']])) { $current_value = array($current_value[$field['row_key']][$field['name']]); } } /// Check if array is multidimensional. if (count($current_value) == count($current_value, COUNT_RECURSIVE)) { /// Do as usual. Array contains only ids. $current_value = $connected_entity ->whereIn($connected_entity_key_name, $current_value) ->get() ->pluck($field['attribute'], $connected_entity_key_name); } else { /// We Have to loop throught array because its array of arrays. /// Then get $field['name'] from array and create array of ids. $arr = array(); foreach ($current_value as $key => $value) { array_push($arr, $value[$field['name']]); } $current_value = $connected_entity ->whereIn($connected_entity_key_name, $arr) ->get() ->pluck($field['attribute'], $connected_entity_key_name); } break;

Thank you @PitchRE 🙏

tabacitu · 2021-12-23T09:02:32Z

@pxpm I tried it, I replicated the bug, but... I'm still not sure I understand your solution, it seems overly complicated to me... And I have to admit, as soon as I heard "we have to change all the fields to account for this key"...

Help me out, here's how I rephrase it in my own words...

if you add a field for a BelongsToMany relation in 4.2, it will use a repeatable
when you submit the main form, but it will fail validation
the old value of the BelongsToMany relation will be wrong (only the first will be kept);

And you're saying that's because the input names are identical for BOTH the main relationship AND the primary key of each entry:

Which makes sense. But here's what I don't understand: when Laravel loads the blade file for the first select2 in that repeatable... isn't it trying to get the old value of dummyproducts[0][dummyproducts]? Isn't that the input name? That's how I would phrase the problem:

the nested select2 is getting the value using old('dummyproducts')
when it should be getting the value using old('dummyproducts')[0]['dummyproducts']; or more specifically, because old() doesn't support dot notation, we should be getting data_get(session()->getOldInput(), 'dummyproducts.0.dummyproduct');

If that's the problem, I agree with you, the solution wouldn't be this one. And it definitely wouldn't be to modify all our fields to account for the repeatable row key. The solution would be to get old('the_right_name'). To change oldValueDefaultOrFallback() in #3054 so that it returns the correct value. BUUUT. Let's please do that using a separate PR, pointing to #3054.

What do you think @pxpm ? Am I understanding this correct? Is this the path forward?

pxpm · 2021-12-23T11:40:30Z

@tabacitu you are totally right in your reasoning.

Just don't focus too much on is a BelongsToMany because we should focus on if a field with the same name of repeatable field exists in the subfields. That is the real problem.

BUT unfortunatelly we have no way to know what is the field name before the field is completely setup.

field is added in controller.

array:19 [▼
  "name" => "dummyproducts"
  "multiple" => false
  "ajax" => false
  "data_source" => "false"
  "minimum_input_length" => 0
  "delay" => 500
  "placeholder" => "Select an entry"
  "entity" => "dummyproducts"
  "label" => "Dummyproducts"
  "relation_type" => "BelongsToMany"
  "model" => "App\Models\Product"
  "attribute" => "name"
  "pivot" => true
  "type" => "relationship_select"
  "row_key" => 0
  "value" => "1"
  "allows_null" => true
  "include_all_form_fields" => true
  "options" => Illuminate\Support\Collection {#1974 ▶}
]

When setupRepeatableElement is called is when we setup the field names, but they have their values already.
When we setup the field we setup the value too, that's how we populate regular repeatable field, because by the order of events: old('repeatable_field') will not exists, and will fallback for the value we setup when setting up the repeatable.

It will fail in scenarios where you have a field with same name inside repeatable and outside repeatable, because old(repeatable_subfield_name) will return old(main_item_field') when they have the same name.

We cannot simply check if it is an array because there are fields that really post an array. So the way I found to distinguish when we should intervene is setting up row_key on the field when setting up the repeatable and check if it is set. If it is we want to get the same field name, but from inside the array submitted.

I also think this should be done inside the oldOrFallback() function without the need to modifiy the fields.

Let me know if you think of other way to deal with this.

Pedro

tabacitu · 2021-12-23T12:54:52Z

My question is... why does repeatable not change the subfield names in PHP instead of JS?

It knows the fields.
It knows the values.
It knows how many rows there should be.

Why does repeatable not use that information to make sure the subfields use "testimonial[name]" instead of "name".

Isn't that possible and/or recommended?

tabacitu · 2021-12-23T12:56:05Z

If repeatable made sure the names are correct (nested with dot or array syntax whatever), then the old value would be populated correctly with the function I just posted in an inline comment in the oldValeDefaultOrFallback() PR.

pxpm · 2021-12-23T13:40:38Z

@tabacitu because some fields change their name in the field itself, like date_range where field setup two fields from the field array name you provide like [start_date, end_date].

We can do a BC in this field and instead of accepting array names we accept only single field name, and add a config in the field to determine to what columns the values should be saved, by default start_date, end_date.

I think it's the only field that accepts an array as field name.

I think if it is not because that field, we can change the names on display. I will have to have a better look also in fields that uses hidden inputs but I think there is no problem there.

Want me to put up a PR with this solution ?

pxpm · 2021-12-23T15:54:58Z

Just created a PR for it: #4032

Does not cover fields that accept arrayed names.

Should work well for majority of fields. Didn't tested 1 by 1.

If that PR gets accepted this should be closed.

tabacitu · 2021-12-23T16:02:54Z

Muuuch better yes 🎉🎉🎉 Thank you @pxpm . Let's close this in favor of polishing #4032

tabacitu · 2021-12-24T06:29:11Z

Does not cover fields that accept arrayed names.

I don't see why this wouldn't work for arrays too... please see https://github.com/Laravel-Backpack/CRUD/pull/4032/files#r774663397

fix old inputs for repeatable

4397314

pxpm changed the base branch from master to 4.2 December 22, 2021 17:30

pxpm mentioned this pull request Dec 22, 2021

[Bug] Field relationship_select causes QueryException when validation fails. #4027

Closed

pxpm assigned tabacitu Dec 22, 2021

pxpm added the Bug label Dec 22, 2021

PitchRE reviewed Dec 22, 2021

View reviewed changes

tabacitu added the Priority: MUST label Dec 23, 2021

tabacitu changed the title ~~Fix old() in repeatable~~ Repeatable subfields not getting the correct old() value for belongsToMany relation Dec 23, 2021

tabacitu assigned pxpm and unassigned tabacitu Dec 23, 2021

pxpm assigned tabacitu and unassigned pxpm Dec 23, 2021

tabacitu changed the title ~~Repeatable subfields not getting the correct old() value for belongsToMany relation~~ Repeatable subfields not getting the correct old() value if there's a field with the same name in the main form Dec 23, 2021

tabacitu changed the title ~~Repeatable subfields not getting the correct old() value if there's a field with the same name in the main form~~ [Bug] Repeatable subfields not getting the correct old() value if there's a field with the same name in the main form Dec 23, 2021

tabacitu closed this Dec 23, 2021

tabacitu deleted the fix-old-for-repeatable branch December 23, 2021 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] Repeatable subfields not getting the correct old() value if there's a field with the same name in the main form #4029

[Bug] Repeatable subfields not getting the correct old() value if there's a field with the same name in the main form #4029

pxpm commented Dec 22, 2021 •

edited

Loading

scrutinizer-notifier commented Dec 22, 2021

PitchRE commented Dec 22, 2021

pxpm commented Dec 22, 2021

PitchRE commented Dec 22, 2021 •

edited

Loading

PitchRE Dec 22, 2021 •

edited

Loading

tabacitu Dec 23, 2021

tabacitu commented Dec 23, 2021 •

edited

Loading

pxpm commented Dec 23, 2021

tabacitu commented Dec 23, 2021

tabacitu commented Dec 23, 2021 •

edited

Loading

pxpm commented Dec 23, 2021

pxpm commented Dec 23, 2021 •

edited

Loading

tabacitu commented Dec 23, 2021

tabacitu commented Dec 24, 2021

[Bug] Repeatable subfields not getting the correct old() value if there's a field with the same name in the main form #4029

[Bug] Repeatable subfields not getting the correct old() value if there's a field with the same name in the main form #4029

Conversation

pxpm commented Dec 22, 2021 • edited Loading

WHY

BEFORE - What was wrong? What was happening before this PR?

AFTER - What is happening after this PR?

HOW

How did you achieve that, in technical terms?

Is it a breaking change or non-breaking change?

How can we test the before & after?

scrutinizer-notifier commented Dec 22, 2021

PitchRE commented Dec 22, 2021

pxpm commented Dec 22, 2021

PitchRE commented Dec 22, 2021 • edited Loading

PitchRE Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

tabacitu Dec 23, 2021

Choose a reason for hiding this comment

tabacitu commented Dec 23, 2021 • edited Loading

pxpm commented Dec 23, 2021

tabacitu commented Dec 23, 2021

tabacitu commented Dec 23, 2021 • edited Loading

pxpm commented Dec 23, 2021

pxpm commented Dec 23, 2021 • edited Loading

tabacitu commented Dec 23, 2021

tabacitu commented Dec 24, 2021

pxpm commented Dec 22, 2021 •

edited

Loading

PitchRE commented Dec 22, 2021 •

edited

Loading

PitchRE Dec 22, 2021 •

edited

Loading

tabacitu commented Dec 23, 2021 •

edited

Loading

tabacitu commented Dec 23, 2021 •

edited

Loading

pxpm commented Dec 23, 2021 •

edited

Loading