Change repeatable names on display #4032

pxpm · 2021-12-23T14:52:55Z

WHY

This will make old() wrapper work for repeatable by using [] arrayed names.

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

Fields didn't overwritte the name in PHP only in JS. This way old() would get the real name eg: category instead of the repeatable correct name repeatable[row][category]

AFTER - What is happening after this PR?

Names are changed before displaying them.

HOW

How did you achieve that, in technical terms?

Setup the name before repeatable initializes.

@tabacitu this seems to fix the problem.

This points to create-old-input-helper branch,

This does not take into account fields with arrayed names.

…n-display

tabacitu · 2021-12-23T16:47:05Z

Pedro... I think we're onto something here.

Please:

test this thoroughly, to see if it will have negative implications on any fields;
let's talk date_range changes, before we do them; what are our options? any chance we can keep both the array names (no breaking change) and this?
if we do this, shouldn't we remove the JS that does this in repeatable? or do we still need that?

pxpm · 2021-12-23T16:53:26Z

let's talk date_range changes, before we do them; what are our options? any chance we can keep both the array names (no breaking change) and this?

We can still have both, but notice that date_range will not work inside repeatable with old() data and would probably break if it happens to have the same name of some other field in the form because date_range name will only change in JS as previously the other fields.

if we do this, shouldn't we remove the JS that does this in repeatable? or do we still need that?

Nop, we still need the JS version that takes care of naming when new item is added or items are moved.

PitchRE · 2021-12-23T17:04:17Z

I pulled this PR and made few tests.
As always, that's my field definition.

        CRUD::addField([
            'name'    => 'characters',
            'label'   => 'Assosciate Characters with this origins.',
            'tab'   => 'Relationships',
            'wrapper' => [
                'class' => 'form-group col-md-6',
            ],

            'pivot_selector' => [
                'wrapper' => [
                    'class' => 'form-group col-md-6',
                ]
            ],
            'pivotFields' => [
                [
                    'name'    => 'as',
                    'type'    => 'text',
                    'wrapper' => [
                        'class' => 'form-group col-md-6',
                    ],
                ],

            ],

            'new_item_label'  => 'Associate Character',
            'init_rows' => 0,
            'min_rows' => 0,
            'max_rows' => 10,
        ]);

When updating (creating works fine)resource, it throws an exception.
( i am not overwriting any view, cleared cache etc.)

Undefined array key "characters"
Illuminate\Foundation\Bootstrap\HandleExceptions::handleError
vendor/backpack/crud/src/app/Library/CrudPanel/Traits/Create.php:119

/// Traits/Create.php:119
if (isset($field['pivotFields'])) {
                    foreach ($values as $pivot_row) {
                        Log::channel('daily')->debug($pivot_row);
                        $relation_data[$pivot_row[$field['name']]] = Arr::except($pivot_row, $field['name']);

                    }

                }

This doesn't seem to be correct.

[2021-12-23 16:58:35] local.DEBUG: array (
  'characters[0' => 
  array (
    'characters' => '2',
    'as' => 'saddasasdasd',
  ),
)

…n-display

pxpm · 2021-12-23T17:54:49Z

Hey @PitchRE , I can't stress enough how important it was for me that you stick with this issue and provided all the help needed to reproduce it! I really appreciate your efforts here. 🙏

I was finally able to reproduce it and I think it's now fixed for good.

I don't wanna ask, but .. wanna try ? 😄

Thanks again, and Merry Christmas!

Pedro

PitchRE · 2021-12-23T18:35:51Z

@pxpm You too! :)
Thank you for work you are putting into this issue.
I executed composer update with "backpack/crud": "dev-change-repeatable-names-on-display as 4.1.99",.

So the problem right now is that updating "kinda works" but removing/attaching relationship doesn't.

change-repeatable-names-on-display.mp4

Logs Here

Logs of `$input` inside `CrudPanel/Traits/Create@syncPivot`

When only updating (not removing, adding relationships

[2021-12-23 18:32:00] local.DEBUG: array (
  'name' => 'aaaaa',
  'characters' => 
  array (
    0 => 
    array (
      'characters' => '2',
      'as' => 'THIS ROW WAS ALREADY THERE 1',
    ),
    1 => 
    array (
      'characters' => '1',
      'as' => 'THIS ROW WAS ALREADY THERE 2',
    ),
  ),
)

When I added a relationship.

[2021-12-23 18:31:23] local.DEBUG: array (
  'name' => 'aaaaa',
  'characters' => 
  array (
    0 => 
    array (
      'characters[0' => 
      array (
        'characters' => '2',
        'as' => 'THIS ROW WAS ALREADY THERE 1',
      ),
    ),
    1 => 
    array (
      'characters[1' => 
      array (
        'characters' => '1',
        'as' => 'THIS ROW WAS ALREADY THERE 2',
      ),
    ),
    2 => 
    array (
      'characters' => '3',
      'as' => '!!!! THIS IS NEW RELATIONSHIP !!!!',
    ),
  ),
)

Very dirty fix to Create@syncPivot

                $relation_data = [];
                if (isset($field['pivotFields'])) {
                    foreach ($values as $pivot_row) {
                        if (!isset($pivot_row[$field['name']])) {

                            $relation_data[reset($pivot_row)[$field['name']]] = Arr::except(reset($pivot_row), $field['name']);
                        } else {


                            $relation_data[$pivot_row[$field['name']]] = Arr::except($pivot_row, $field['name']);
                        }
                    }
                }

So with this patch, it works for me. But that's not the point.
Why, when adding/removing relationship, old relationship keys look like this characters[0, characters[1?
Is that intended? If yes, why?

tabacitu · 2021-12-23T16:09:34Z

src/resources/views/crud/fields/inc/repeatable_row.blade.php

@@ -27,6 +27,7 @@
                    if(isset($row[$subfield['name']]) || isset($row[$subfield['name'].'[]'])) {
                        $subfield['value'] = $row[$subfield['name']] ?? $row[$subfield['name'].'[]'];
                    }
+                    $subfield['name'] = $field['name'].'['.$repeatable_row_key.']['.$subfield['name'].']';


Two questions here, both might be absurd or stupid:

What if subfield['name'] is already a bracket-using-field-name? Can there ever be?

What if $subfield is another repeatable?

I guess question no 2 also implies question no 1, since repeatable will have subfields. And I know we said we don't support repeatable inside repeatable (I still think it's too complicated to support right now). But... now that relationship loads repeatable for some relationships... we have to at least know what will happen... If not all relationships can be used inside repeatable... we have to know that...

Third question. Can't we check if it's an array here?

if (is_string($subfield['name']) { $subfield['name'] = $field['name'].'['.$repeatable_row_key.']['.$subfield['name'].']'; } elseif (is_array($subfield['name'])) { // probably a date_range but hey, who knows, might be somthing else too foreach ($subfield['name'] as $k => $item) { $subfield['name'][$k] = $field['name'].'['.$repeatable_row_key.']['.$item['name'].']'; } // and if (for some reason) we can't do that... we can even do something hacky like $subfield['some_flag_for_the_field_to_know_its_repeatable'] = true; }

Hey @tabacitu

1 - Can never be.
2 - If subfield is another repeatable it will have the repeatable name there.
3 - Not even considering that for now, after the "first repeatable" it's initialized the "repeatable" field name is changed to accomodate "first repeatable row", so imagine the following scenario:

$this->crud->addFields([ [ 'name' => 'extras', 'type' => 'repeatable', 'fields' => [ [ 'name' => 'testing', 'type' => 'repeatable', 'fields' => [ [ 'name' => 'rep sub 1', ] ] ], [ 'name' => 'testing2' ] ] ],

When extras initialize, the testing repeatable name would become: extras[row][testing] breaking the nested repeatable.

Like you said, I didn't focused on making repeatable work inside repeatable. It would probably be doable since we now use array names, but need a big refactor on how repeatable initializes itself and nested fields.

I've fixed the issue with names in JS (where was the problem for regular/relationship repeatables) and added your suggestion to parse the field names when array.

Ok, I understand why it wouldn't work. As I said... I still think it's NOT important to support repeatable inside repeatable.

But now that some of the relationship fields use repeatable... we have to know. What combinations will NOT work? From what I understand... the new relationships that 4.2 brings will not work inside repeatable right? Which begs the question... does it make sense to use any relationship field inside repeatable??! Not really... right? You only want to use relationship to update the relationship on the Model... when you want a relationship inside the repeatable... you're better off using select_from_array or select_from_ajax, right?

If anything, we should at least add them to the "relationship edge cases" issue - #4012

@tabacitu exactly.

I don't think there is any sense in using relationships inside repeatable.

One thing is using repeatable to edit a relation (like we do), other thing is using repeatable field on your own and add relationship fields there. The CrudField will be the repeatable, not any of the repeatable fields.

The only reason people use relationships inside repeatable is because the setup of the fields is easier since relationships support adding only the field name as a relation and backpack does a great job guessing the stuff needed.

It will show the "relationship" field, but technically it will never be a relationship, it's just a select.

Like I told you earlier, it's way easier to support repeatable inside repeatable now. I am sure that if I parsed the names a little bit more I would endup making it work ... Maybe without BC, so we can have a look at it in 4.2.xx !

…table

tabacitu · 2022-01-03T07:37:32Z

@pxpm , what's the status on this? How can I test the before&after?

Thanks!

pxpm · 2022-01-03T11:02:25Z

@tabacitu this is ready to merge.

This was pointing to old() branch because it relates with.

Before repeatable names would only be setup with JS in repeatable initialization. This change makes all fields get the repeatable names before they reach the page otherwise some_field would not exist in old as in old it is: repeatable[row]some_field.

To test this before and after you can add a repeatable field and purposely fail the validation of some field inside repeatable.

Let me know,
Pedro

tabacitu · 2022-01-03T16:43:14Z

Hmm... still cannot confirm the bug is there on the 4.2 branch. Here's what I did:

went to Dummy CRUD
tried to create an entry with everything filled in, but without the Simple "text"
validation failed, because "text" inside "simple" is required|min:10|max:255
✅ all fields (inside and outside repeatable) are populated with the old entries
✅ everything seems to work fine 🤷‍♂️

Why is this needed again?

pxpm · 2022-01-05T11:24:10Z

@tabacitu sorry if I wasn't 100% clear.

The problem here is the fact that if we don't change the names before parsing the field blade file, the $field['name'] will be the one setup in crud, and not the repeatable one.

Follow me, dummyproducts is a BelongsToMany relation with aditional pivot fields: (it's in Monster in demo 4.2, you can try it out there).

$this->crud->addFields([
            [
                'name' => 'dummyproducts',
                'pivotFields' => [
                    [
                        'name' => 'notes',
                        'type' => 'textarea'
                    ],
                ],
            ],
        ]);

This will add a repeatable_relation, that automatically adds a pivot field with the same name of the relation, in this case dummyproducts.

If we don't change the field name before displaying the field, $field['name'] that we use to call the old(), would be dummyproducts instead of dummyproducts[0]dummyproducts, dummyproducts[1]dummyproducts etc etc. So in this example as you can see from the following recording: https://recordit.co/I5zUbdyxqw when the validation fails, both of them get the same value because old() is called with the original field name making it behave wrong.

Let me know if I made myself clear now.

Thanks,
Pedro

or any other fields with multiple names

tabacitu · 2022-01-05T13:42:29Z

Oh ok, I get it now, thanks for the explanation and video @pxpm . So we're basically fixing a bug in repeatable_relation, by making repeatable and repeatable_row a bit broader, I understand now 🙏

I can confirm this is happening BEFORE this PR! I can confirm this is no longer happening AFTER this PR! 🎉🎉🎉
I can also report this broke the Dummy CRUD. If you filled in all fields and clicked Save & Edit you got the error below after the successful save (when reaching admin/dummy/3/edit). I suspected it's because of the date_range field. I've drilled down and noticed a few errors when rewriting the names and values. I think I've tracked down the problem and fixed it on this branch; after my change, date_range is now working for me, both inside repeatable and outside it (both with old and normal value). But please PLEASE triple-check me. My commit is here

I could NOT reproduce @PitchRE 's bug (reported here); not the way I understand it; for me, creating & updating & removing seem to work fine (see video here - https://www.loom.com/share/e053f5357e2e44909acb3db41ee4040a); @pxpm can you please read his report, understand and test it yourself and decide if there's a bug here? if not... what the hell happened for him?!

Otherwise, I cannot find a fault with this PR and I'm inclined to merge it.

Cheers!

pxpm · 2022-01-06T10:43:57Z

@tabacitu I've just checked your fix for date_range and it seems to work just fine. My only concern is that we are adding that conditional fixing into the "core" only to solve this field problem, that we would probably be better fixing in the field itself but conventionally removing the support for arrayed field names.

This should become field configurations instead of field name defining what other fields that field is going to setup.

But I get if you don't wanna mess with that rn. But maybe leave a comment in the code about it so we don't forget ?

About the @PitchRE problem I was able to reproduce it, and fix it. If you look close at the reported results the problem was repeatable setting up the names wrong leaving "[" where it shouldn't. You cannot replicate it because @PitchRE tested this branch in the middle, so it's already fixed on top of it.

Let me know,
Pedro

tabacitu · 2022-01-06T12:09:31Z

About the @PitchRE problem I was able to reproduce it, and fix it.

Awesome. Let's merge this bad boy!

My only concern is that we are adding that conditional fixing into the "core" only to solve this field problem, that we would probably be better fixing in the field itself but conventionally removing the support for arrayed field names. This should become field configurations instead of field name defining what other fields that field is going to setup. But I get if you don't wanna mess with that rn.

Yeah.... I'm a bit unconfortable with that rn. Let's leave it for future-us. 😅👀

pxpm added 3 commits December 23, 2021 14:42

refactor

9b91c78

Merge branch 'create-old-input-helper' into change-repeatable-names-o…

d26c462

…n-display

fix for old, add repeatable name on display

d5938a1

pxpm assigned tabacitu Dec 23, 2021

pxpm added the Bug label Dec 23, 2021

This was referenced Dec 23, 2021

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

Closed

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

Closed

tabacitu added the Priority: MUST label Dec 23, 2021

tabacitu removed their assignment Dec 23, 2021

pxpm assigned tabacitu and unassigned tabacitu Dec 23, 2021

pxpm added 2 commits December 23, 2021 17:39

Merge branch 'create-old-input-helper' into change-repeatable-names-o…

cda5db8

…n-display

fix for repeatable container names

ef985b8

tabacitu reviewed Dec 24, 2021

View reviewed changes

tabacitu assigned pxpm Dec 24, 2021

pxpm added 3 commits December 27, 2021 11:00

Merge branch '4.2' into change-repeatable-names-on-display

0a7be5b

repeatable name update takes into account name could already be repea…

9b55b3a

…table

account for arrayed field names

3939a16

Base automatically changed from create-old-input-helper to 4.2 December 31, 2021 06:06

Merge branch '4.2' into change-repeatable-names-on-display

d17a29a

fix date_range names rewrite inside repeatable_row

47afad5

or any other fields with multiple names

tabacitu merged commit 8a80096 into 4.2 Jan 6, 2022

tabacitu deleted the change-repeatable-names-on-display branch January 6, 2022 12:09

tabacitu mentioned this pull request Jan 6, 2022

[WIP] Version 4.2 #2940

Closed

tabacitu mentioned this pull request Feb 4, 2022

v5 #4152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change repeatable names on display #4032

Change repeatable names on display #4032

pxpm commented Dec 23, 2021 •

edited

Loading

tabacitu commented Dec 23, 2021 •

edited

Loading

pxpm commented Dec 23, 2021 •

edited

Loading

PitchRE commented Dec 23, 2021

pxpm commented Dec 23, 2021

PitchRE commented Dec 23, 2021 •

edited

Loading

tabacitu Dec 23, 2021

tabacitu Dec 24, 2021

pxpm Dec 27, 2021

tabacitu Dec 28, 2021

tabacitu Dec 28, 2021

pxpm Dec 28, 2021

tabacitu commented Jan 3, 2022 •

edited

Loading

pxpm commented Jan 3, 2022

tabacitu commented Jan 3, 2022

pxpm commented Jan 5, 2022

tabacitu commented Jan 5, 2022 •

edited

Loading

pxpm commented Jan 6, 2022

tabacitu commented Jan 6, 2022

Change repeatable names on display #4032

Change repeatable names on display #4032

Conversation

pxpm commented Dec 23, 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?

tabacitu commented Dec 23, 2021 • edited Loading

pxpm commented Dec 23, 2021 • edited Loading

PitchRE commented Dec 23, 2021

pxpm commented Dec 23, 2021

PitchRE commented Dec 23, 2021 • edited Loading

Very dirty fix to Create@syncPivot

tabacitu Dec 23, 2021

Choose a reason for hiding this comment

tabacitu Dec 24, 2021

Choose a reason for hiding this comment

pxpm Dec 27, 2021

Choose a reason for hiding this comment

tabacitu Dec 28, 2021

Choose a reason for hiding this comment

tabacitu Dec 28, 2021

Choose a reason for hiding this comment

pxpm Dec 28, 2021

Choose a reason for hiding this comment

tabacitu commented Jan 3, 2022 • edited Loading

pxpm commented Jan 3, 2022

tabacitu commented Jan 3, 2022

pxpm commented Jan 5, 2022

tabacitu commented Jan 5, 2022 • edited Loading

pxpm commented Jan 6, 2022

tabacitu commented Jan 6, 2022

pxpm commented Dec 23, 2021 •

edited

Loading

tabacitu commented Dec 23, 2021 •

edited

Loading

pxpm commented Dec 23, 2021 •

edited

Loading

PitchRE commented Dec 23, 2021 •

edited

Loading

tabacitu commented Jan 3, 2022 •

edited

Loading

tabacitu commented Jan 5, 2022 •

edited

Loading