Skip to content
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

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

Closed
PitchRE opened this issue Dec 22, 2021 · 18 comments
Closed
Assignees
Labels
Possible Bug A bug that was reported but not confirmed yet. triage

Comments

@PitchRE
Copy link

PitchRE commented Dec 22, 2021

Bug report

Refs;
Probably related to this PR: #4002

What I did

Created/Updated resource that contains values inside relationship_select field and validated request.
Validation rules doesn't have anything to do with relationship_select CRUD field.

Field Definition

        CRUD::addField([
            'name'    => 'origins',
            'label'   => 'Assosciate 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',
                    ],
                ],

            ],

What I expected to happen

If validation passes:

  • Create/Update Resource

If validation fails:

  • Redirect back with validation errors being displayed.

What happened

Illuminate\Database\QueryException
SQLSTATE[HY093]: Invalid parameter number (SQL: select * from `origins` where `id` in (2)) (View: /xxx/xxx/xxx/vendor/backpack/crud/src/resources/views/crud/fields/relationship/relationship_select.blade.php)

This only occurs when validation fails and relationship_select field has any data.

Is it a bug in the latest version of Backpack?

After I run composer update backpack/crud the bug... is it still there?

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is:

### PHP VERSION:
PHP 8.0.13 (cli) (built: Nov 22 2021 09:50:43) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.13, Copyright (c) Zend Technologies
    with Zend OPcache v8.0.13, Copyright (c), by Zend Technologies

### LARAVEL VERSION:
v8.77.0@148b0df9eaac9f89bebb6b6fbbb0e9874e62ecc7

### BACKPACK VERSION:
4.2.x-dev@69d7047ae60a2c45a0cd02c9e56553f42e961d72
@PitchRE PitchRE added the triage label Dec 22, 2021
@welcome
Copy link

welcome bot commented Dec 22, 2021

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication channels:

  • Bug Reports, Feature Requests - Github Issues (here);
  • Quick help (How do I do X) - Gitter Chatroom;
  • Long questions (I have done X and Y and it won't do Z wtf) - Stackoverflow, using the backpack-for-laravel tag;
  • Showing off something you've made, asking for opinion on Backpack/Laravel matters - Reddit;

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

--
Justin Case
The Backpack Robot

@pxpm
Copy link
Contributor

pxpm commented Dec 22, 2021

On it. Thanks for the report @PitchRE !

I've just tested the relationship_select as a single field and can confirm this does not happen due to relationship_select itself probably only when used as a pivot selector.

Be back in a few!

@pxpm
Copy link
Contributor

pxpm commented Dec 22, 2021

Hey @PitchRE

Thanks again for taking time to submit the report. I think I found the culprit.

I've submitted PR #4029 to address this issue. Please notice that the PR needs review and we are not sure yet if this is the correct/optimal way to fix this, but if you want to give it a try and let us know if it fixed for you, it could help us alot.

I am going to close this since we have the PR, but feel free to comment here or re-open if the PR didn't fixed it for you.

Wish you the best,
Pedro

@pxpm pxpm closed this as completed Dec 22, 2021
@pxpm pxpm reopened this Dec 22, 2021
@pxpm
Copy link
Contributor

pxpm commented Dec 22, 2021

Let's get back here @PitchRE !

Clearly not the same issue as in the other PR. I cannot reproduce your issue so .. let's try it again.

Can you share with me your pivot table ? I will try to setup exactly the same scenario as you.

@PitchRE
Copy link
Author

PitchRE commented Dec 22, 2021

@pxpm Please take a look at this repository. I just created brand new laravel project where I reproduced this issue.
Unfortunately I forgot to add some directories to gitignore so commit is a mess.

@pxpm
Copy link
Contributor

pxpm commented Dec 22, 2021

Hey @PitchRE

Thanks again for your support helping to fix this issue. I am still not able to reproduce it.

Here is a .gif of me playing with your repository: https://recordit.co/diqyWa58xQ

Can you give me the exact steps to reproduce your issue ?

Best,
Pedro

@PitchRE
Copy link
Author

PitchRE commented Dec 22, 2021

@pxpm Damn, I am speechless. If you cloned my repository, you for sure should be able to reproduce it.
Which database engine are you using?

Please take a look at #4029 (comment)

@pxpm
Copy link
Contributor

pxpm commented Dec 23, 2021

@PitchRE Hey there!

Thanks for the submitted code. Unfortunatelly that would only apply to the selects fields and this is a general problem of using the combination with repeatable.

Can you make sure you don't have the backpack files in your resources/views/vendor/backpack/crud and are overwritting them? When they are present in that folder backpack will use that file and never pick the updated file from backpack source.

I don't see what could be if not that. Try clearing your caches php artisan view:clear && php artisan cache:clear

Let me know,
Pedro

@PitchRE
Copy link
Author

PitchRE commented Dec 23, 2021

@pxpm

Can you make sure you don't have the backpack files in your resources/views/vendor/backpack/crud and are overwritting them?

Yes, I am sure. Currently testing on fresh laravel project. PitchRE/issue-4027

4027.mp4

I don't think I made any mistakes in relationship/field definition. It's only few files so you can verify that on that repo.

Snippet I inserted bellow seems to fix it in my case.
Although I am not sure about it, it might break in different cases ( I don't actually know how it works under the hood...).

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']]);
}
}
Log::channel('daily')->debug($current_value);
/// 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;

These are just my loose thoughts.

So why sometimes $current_value is an multidimensional array and other times not?
What's the intended behaviour of $current_value?

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

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

In this case it will work because $current_value is just simple array array(1,5,7) ;

[2021-12-23 14:39:24] local.DEBUG: array (
  0 => '1',
  1 => '4',
)  

But as soon as I change field definition to this, it breaks

        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,
        ]);

And Log output is way different.


[2021-12-23 14:41:08] local.DEBUG: array (
  0 => '1',
)  
[2021-12-23 14:41:08] local.DEBUG: array (
  0 => '4',
)  
[2021-12-23 14:41:08] local.DEBUG: array (
  0 => 
  array (
    'characters' => '1',
    'as' => 'test char',
  ),
  1 => 
  array (
    'characters' => '4',
    'as' => 'fuuuu char',
  ),
)  

This multidimensional array. I guess It shouldn't be there at all ?
Because if I just return null when $current_value is an multidimensional array, I don't see any changes in behaviour of the field.

@pxpm
Copy link
Contributor

pxpm commented Dec 23, 2021

@PitchRE that is due to the fact that the pivot selector is called characters and the main field, the repeatable field, is also called characters. So when getting from old() it gets the main field characters, that contains the multiple pivot fields that are also called characters.

#4032 should probably fix this without needing this fix in the field files.

@pxpm
Copy link
Contributor

pxpm commented Feb 3, 2022

This is fixed I think.

Please re-open or comment if I am wrong.

Thanks for testing 4.2 @PitchRE , since this issue was opened alot of stuff have been added/fixed regarding relationships specially in the last days with nested relations.

Let us know if you find something broken while using that branch :)

Pedro

@pxpm pxpm closed this as completed Feb 3, 2022
@csathoPeter
Copy link

Hey @pxpm, I can confirm that this issue still exists with relationship fields which includes subfields (pivot columns), with latest backpack/crud and backpack/pro

@csathoPeter
Copy link

Adding
$pivotSelectorField['name'] = $field['name'].'-subfield';
to vendor/backpack/crud/src/app/Library/CrudPanel/Traits/Relationships.php:291 seems to fix the issue

@tabacitu tabacitu reopened this Feb 17, 2022
@tabacitu tabacitu changed the title [Bug] [4.2.x-dev] Field relationship_select causes QueryException when validation fails. [Bug] Field relationship_select causes QueryException when validation fails. Feb 17, 2022
@tabacitu
Copy link
Member

@csathoPeter are you getting this when using the 4.2 branch, or after upgrading to v5?

@csathop-btu
Copy link

csathop-btu commented Mar 16, 2022 via email

@pxpm
Copy link
Contributor

pxpm commented Mar 16, 2022

@csathop-btu or @csathoPeter 👍

Thanks, can you provide me the output of php artisan backpack:version and also your field definition ?

Thanks

@tabacitu tabacitu added Possible Bug A bug that was reported but not confirmed yet. and removed Priority: MUST Bug labels Apr 4, 2022
@tabacitu
Copy link
Member

tabacitu commented Aug 3, 2022

Bump! Can anybody please give us more information on this? Would love to get to the bottom of this - are you still seeing this issue?

@pxpm
Copy link
Contributor

pxpm commented Aug 3, 2022

@tabacitu I think this is fixed for long time now.

I am going to close and re-open if someone pings here.

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Possible Bug A bug that was reported but not confirmed yet. triage
Projects
None yet
Development

No branches or pull requests

5 participants