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] Save relationship data using DB transaction #5401

Closed
YosraHamza opened this issue Dec 9, 2023 · 4 comments
Closed

[Bug] Save relationship data using DB transaction #5401

YosraHamza opened this issue Dec 9, 2023 · 4 comments

Comments

@YosraHamza
Copy link

Bug report

What I did

I added a relationship field to my form, saved the form with its relation. One of the relations threw an error from the database (field cannot be null).

What I expected to happen

All the data will not be stored in the database. Main entity data and its relations.

What happened

The main entity data were saved without the relations

What I've already tried to fix it

The original function needs to use DB transactions in the store/update function

Is it a bug in the latest version of Backpack?

Yes

Backpack, Laravel, PHP, DB version

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

backpack/basset: 1.2.2
backpack/crud: 6.3.0
backpack/generators: v4.0.2
backpack/logmanager: v5.0.1
backpack/pro: 2.0.18
backpack/settings: 3.1.0
backpack/theme-coreuiv2: 1.2.2
backpack/theme-tabler: 1.1.1
@phpfour
Copy link
Contributor

phpfour commented Dec 9, 2023

Hey @YosraHamza

We need more info from you to reproduce the issue you're facing so that we can look for a possible solution.

See here for an example of a good issue with necessary details.

Thanks!

@phpfour phpfour self-assigned this Dec 9, 2023
@phpfour phpfour moved this to Needs Testing, Review or Docs in This week Dec 9, 2023
@YosraHamza
Copy link
Author

Hello @phpfour

Here's an example of the issue:

$this->crud->field('title')->size(6);
$this->crud->field('branch')->type('text')->size(6);
$this->crud->field('items')->type('relationship')
        ->subfields([
            [
                'name' => 'name',
                'type' => 'text',
                'wrapper' => [ 'class' => 'form-group col-md-4' ],
            ],
            [
                'name' => 'amount',
                'type' => 'number',
                'wrapper' => [ 'class' => 'form-group col-md-4' ],
            ],
            [
                'name' => 'quantity',
                'type' => 'number',
                'wrapper' => [ 'class' => 'form-group col-md-4' ],
            ]
]);

Here, as an example. I have an order with 1-many items.
If somehow, one of the items has an error and the database threw it (for example empty name, or long name that exceeded the column limits) and this error passed from the application validation. I found that the order was saved to the database along with the previous correct items.

Expected behavior:
The order and the items will be saved together, or fail together. As now we have an incomplete order created on the system. And the creator will not notice it when he saw the error page.

This can be fixed if we wrap the create function with the database transaction operation in vendor/backpack/crud/src/app/Library/CrudPanel/Traits/Create.php

Current:

$item = $this->model->create($directInputs);
$this->createRelationsForItem($item, $relationInputs);

New:

DB::transaction(function () use ($directInputs, $relationInputs){
     $item = $this->model->create($directInputs);
     $this->createRelationsForItem($item, $relationInputs);
});

So we can keep the saved data always consistent.

Thanks in advance.

@pxpm
Copy link
Contributor

pxpm commented Dec 11, 2023

Hey @YosraHamza thanks for the suggestion.

I think it makes sense, yes. 👍

Not 100% sure about all the implications (I mean, pitfall in other rdms like nosql,), so I talked with my colleges and we decided that we can push something like this, but it needs to be behind a feature flag.

I am polishing the PR with the changes and I will have it ready either today or tomorrow and we can review if something is missing.

Thanks again for the suggestion.

🙏

@pxpm pxpm assigned pxpm and unassigned phpfour Dec 11, 2023
@pxpm pxpm moved this from Needs Testing, Review or Docs to In Progress in This week Dec 11, 2023
@pxpm
Copy link
Contributor

pxpm commented Mar 29, 2024

This is already done a few versions ago.
https://backpackforlaravel.com/docs/6.x/crud-how-to#enable-database-transactions-for-create-and-update-1

Thanks for the suggestion @YosraHamza

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

No branches or pull requests

3 participants