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] Unable to save empty input with relation HasOne #3944

Closed
CinexUA opened this issue Nov 18, 2021 · 7 comments
Closed

[Bug] Unable to save empty input with relation HasOne #3944

CinexUA opened this issue Nov 18, 2021 · 7 comments
Assignees

Comments

@CinexUA
Copy link

CinexUA commented Nov 18, 2021

Bug report

What I did

Sometimes it is necessary to delete data from input with relation HasOne

I try to store at least one input blank from the list:

$this->crud->addFields([
            [
                'name' => 'userInfo.first_name',
                'label' => __('First name'),
                'type' => 'text',
                'tab' => __('User info'),
                'wrapper' => [
                    'class' => 'form-group col-xs-12 col-sm-12 col-md-6 col-lg-6 col-xl-3',
                ],
            ],[
                'name' => 'userInfo.last_name',
                'label' => __('Last name'),
                'type' => 'text',
                'tab' => __('User info'),
                'wrapper' => [
                    'class' => 'form-group col-xs-12 col-sm-12 col-md-6 col-lg-6 col-xl-3',
                ],
            ],[
                'name' => 'userInfo.hrp_number',
                'label' => __('HRP - number'),
                'type' => 'text',
                'tab' => __('User info'),
                'wrapper' => [
                    'class' => 'form-group col-xs-12 col-sm-12 col-md-6 col-lg-6 col-xl-3',
                ],
            ],[
                'name' => 'userInfo.approved_hrp',
                'label' => __('HRP approved'),
                'type' => 'select_from_array',
                'tab' => __('User info'),
                'options' => UserInfo::getStatusValues(),
                'allows_null' => false,
                'default' => UserInfo::STATUS_APPROVE_PENDING,
                'wrapper' => [
                    'class' => 'form-group col-xs-12 col-sm-12 col-md-6 col-lg-6 col-xl-3',
                ],
            ],[
                'name' => 'userInfo.address',
                'label' => __('Address'),
                'type' => 'text',
                'tab' => __('User info'),
                'wrapper' => [
                    'class' => 'form-group col-xs-12 col-sm-12 col-md-6 col-lg-6',
                ],
            ],[
                'name' => 'userInfo.private_mobile',
                'label' => __('Private mobile'),
                'type' => 'text',
                'tab' => __('User info'),
                'wrapper' => [
                    'class' => 'form-group col-xs-12 col-sm-12 col-md-6 col-lg-6 col-xl-3',
                ],
            ],[
                'name' => 'userInfo.date_of_birth',
                'label' => __('Birthday'),
                'type' => 'date',
                'format' => 'd/m/Y',
                'tab' => __('User info'),
                'wrapper' => [
                    'class' => 'form-group col-xs-12 col-sm-12 col-md-6 col-lg-6 col-xl-3',
                ],
            ],
        ]);

What I expected to happen

I expected to see an empty field when I delete data from it

What happened

Data in input can only be written, changed but not deleted. I want to be able to delete data from the input field.

What I've already tried to fix it

I was looking for a solution on the Internet, in the official documentation, stackowerflow and other resources, but could not find anything

Is it a bug in the latest version of Backpack?

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

Backpack, Laravel, PHP, DB version

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

PHP VERSION:

PHP 8.0.10 (cli) (built: Sep 3 2021 17:04:26) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.10, Copyright (c) Zend Technologies

LARAVEL VERSION:

v8.70.2@dec9524cd0f9fa35a6eb8e25d0b40f8bbc8ec225

BACKPACK VERSION:

4.1.60@bec5cfa11f0cd0712c79256748acf5850080c0ee

@CinexUA CinexUA added the triage label Nov 18, 2021
@welcome
Copy link

welcome bot commented Nov 18, 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 pxpm added Bug and removed triage labels Nov 19, 2021
@pxpm
Copy link
Contributor

pxpm commented Nov 19, 2021

Hey @tabacitu

I was totally aware of this. And I already tried to fix it, but it would be a major BC.

Problem is here:

if (! is_null(Arr::get($data, $attributeKey)) && isset($relation_field['pivot']) && $relation_field['pivot'] !== true) {

The check for is_null removes the field hasOne.some_field from the relation data if empty because the data already went through ConvertEmptyStringsToNull .

On submission:
image

On controller:
image

This is connected with the saving process for multiple entries somehow, because I broke it when trying to fix this and we needed to revert the PR.

In relationship branch I removed this is_null check, but then is necessary to add to all select multiples either the switch between single and multiple to be posted OR since we have repeatable as array now, I think we can go back to the old old trick of having an hidden input that gets sent when the the field is cleared. We would then need to remove the fields that are not in the request from saving so people can use disabled fields, and they would not be updated when not present in the request.

The solution I've found goes along this lines:

private function getRelationDataFromFormData($data)
    {
        $relation_fields = $this->getRelationFields();

        $relation_fields = $this->parseRelationFieldNamesFromHtml($relation_fields);

        //remove fields that are not in the submitted form.
        $relation_fields = array_filter($relation_fields, function ($item) use ($data) {
            return Arr::has($data, $item['name']);
        });

        $relationData = [];
        foreach ($relation_fields as $relation_field) {
            $attributeKey = $relation_field['name'];
            if (isset($relation_field['pivot']) && $relation_field['pivot'] !== true) {

Let me know what you think. I think we cannot fix it without BC because we cannot run the inverse of ConvertEmptyStrings to make it "empty string" again because we would not know what fields in the data should be converted and what should not.

Removing the is_null leads to another BC, and the fix is another BC.

Pedro

@tabacitu
Copy link
Member

Aahh 😡 It's that damn ConvertsEmptyStringsToNull middleware again... 😅

Ok so... here's a wild idea @pxpm ... what if we... skip that middleware when we want to 👀 ? Previously that was impossible (or at least very difficult), but starting with Laravel 8.36 (april 2021), the middleware has a skipWhen() method. I've put together PR #3950 - let me know what you think about it there @pxpm

This is either a brilliant idea, or a very stupid one. Can't figure out which one it is at the moment... 😅

@tabacitu tabacitu assigned pxpm and unassigned tabacitu Nov 22, 2021
@kiddtang
Copy link
Contributor

kiddtang commented Nov 30, 2021

@CinexUA another idea that I workaround for this issue.
As I know this field can be cleared, I reverse ConvertsEmptyStringsToNull with the method

protected function handleEmptyInput($request)
{
    if (is_null($request->input('profile.address_2'))) {
        $profile = $request->input('profile');
        $profile['address_2'] = '';
        $request->request->set('profile', $profile);
    }
    return $request;
}

Inspired by handlePasswordInput, in UserCrudController.php
I tried $request->request->set('profile.address_2', ''); but it is not working. So, I just working on the array.

Apply it like handlePasswordInput

public function update()
{
    $this->crud->setRequest($this->crud->validateRequest());
    $this->crud->setRequest($this->handlePasswordInput($this->crud->getRequest()));
    $this->crud->setRequest($this->handleEmptyInput($this->crud->getRequest()));
    $this->crud->unsetValidation(); // validation has already been run

    return $this->traitUpdate();
}

Hope it helps :-)

@tabacitu @pxpm
Maybe another field attribute allows_null or allow_clear to tell which field needs to reverse ConvertsEmptyStringsToNull ?
Just a wild idea. XD

@tabacitu
Copy link
Member

tabacitu commented Jan 12, 2022

Heads-up - this has been fixed in #4066

Starting with Backpack 4.2, you can use HasOne and MorphOne relationships in two ways:

// add text/number/etc fields for each related attribute
CRUD::field('userInfo.first_name');
CRUD::field('userInfo.last_name');

// add one field for all attributes (it will load a repeatable field with one item behind the scenes)
CRUD::field('userInfo')->subfields([
    ['name' => 'first_name', 'type' => 'text'],
    ['name' => 'last_name', 'type' => 'text'],
]);

The first method will NOT allow the admin to ever delete the related entry. Just update it.
But the second method will. Just click "X" on that entry and Backpack will be delete it from the db.

🎉 So I'll be closing this. If after the 4.2 upgrade you're still having this issue, please reply here again, we'll re-open and re-investigate.

@CinexUA
Copy link
Author

CinexUA commented Mar 1, 2022

Hi! When will laravel backpack 4.2 update be released? I don't want to upgrade to version 5.0 just to avoid this bug.

@tabacitu
Copy link
Member

tabacitu commented Mar 1, 2022

Hi @CinexUA ,

In short, Backpack v4.2 == Backpack 5.0. But it should be a free upgrade for you, if you purchased Backpack after Feb 9th 2021. You can read more about that here. And the upgrade process itself shouldn't be a long one at all.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants