thedevdojo / voyager

Voyager - The Missing Laravel Admin
https://voyager.devdojo.com
MIT License
11.8k stars 2.67k forks source link

many relationship #706

Closed emsici closed 6 years ago

emsici commented 7 years ago

Description:

Tables: certifications - id, name, description clients - certifications_id (multiple select), name, etc

I have created a BREAD for this, but i can't add or edit multiple values from certifications table.

The BREAD is { "relationship": { "key": "id", "label": "name" } }

Client.php

class Client extends Model { protected $table = 'clients';

public function certifications_id() { return $this->belongsTo(Certification::class); }

}

mySQL: clients:

id int(10) unsigned Auto Increment
type varchar(255)
name varchar(255)
created_at timestamp NULL
updated_at timestamp NULL

certifications: id int(10) unsigned Auto Increment
name varchar(255)
indicativ varchar(255)
certifications_id varchar(255)
created_at timestamp NULL
updated_at timestamp NULL

Steps To Reproduce:

snip

snip2

emsici commented 7 years ago

There will be a fix in next release or can i do something to fix this?

Frondor commented 7 years ago

Remember than after #638 you have to name your relationship methods in camelCase, not snake_case anymore. I wonder how your code goes that far, it should fail to resolve the relationship if you're using 0.10.13.

emsici commented 7 years ago

Relations are working, but i can't insert to mysql because it says empty value, i have camelCase relations at this moment.

Frondor commented 7 years ago

I've seen this error once but I use validation for each field. The ones that you can allow to be null, simply uses the nullable rule. Hence, the required ones need that one rule. But to be honest, I can't reproduce it right now at 0.10.13, I don't know about @marktopper

EDIT: If that field can be null, then you have to specify ->nullable() in your migration file, and then write the validation rules to allow null values. If that's not the case, then you also need to write validation logic and make it required

marktopper commented 7 years ago

Haven't been able to reproduce this either...

emsici commented 7 years ago

1 2 3 4 5 6

if i use nullable() > Call to undefined method Illuminate\Database\Query\Builder::sync() when adding

Frondor commented 7 years ago

Now that's a new error.. Can you edit https://github.com/the-control-group/voyager/blob/master/resources/views/bread/browse.blade.php#L40 and use on the previous line dd($data->{$row->field}) to see what value you're getting?

emsici commented 7 years ago

1 2 3 4 5 6 7 8 9

In {{dd($data->{$row->field})}} resources/views/vendor/voyager/client/browse.blade.php? or where? Yes i can, but give me more info, please.

Made that column and bread from scratch and still no success.

Got Slack or something like that?

Frondor commented 7 years ago

mmm.... to be honest, I think that's working as intended. You're supposed to use validation anyway. I'm already addressing other things, so at the moment my mind can't handle this as well :D

marktopper commented 7 years ago

Just a note, it should properly be public function certificationsId().

emsici commented 7 years ago

Look where i add a new client, it says it needs to be certifications_id.

Validate what? i have no data like error says :)

marktopper commented 7 years ago

@emsici: I see, can you please try to update to version 0.10.14 and test it again?

Frondor commented 7 years ago

I can finally reproduce this. @marktopper

Happens to me with a image field which hasn't the current DB value (when editing). The constraint is fine, those fields can not be null, I think the problem is when voyager prepares the model to be saved, It seems to be ignoring database values, and if they weren't included in the request, they are getting overridden to NULL.

I think this is already fixed for select_multiple and select_dropdown fields, since they already pick up the actual value from DB (when editing), unlike image field. But it isn't here where we have to address the error in my opinion;

I think the problem is when voyager prepares the model to be saved, It seems to be ignoring database values, and if they weren't included in the request, they are getting overridden to NULL.

So I have 3 ideas atm

  1. We need to revamp the image field input to work directly with media manager and save strings instead of files (let the media manager do that).
  2. Check which other field types are triggering this error.
  3. This error has nothing to do with many to many relationships I believe, so... Let's set it to something like SQL Error: Field doesn't have a default value so we may get more feedback

EDIT: Now I think this is more a developer error than voyager itself. It won't happen at all if a default value (if it uses one) is provided at the moment of migrating the schema to DB, or a proper validation is set up using details JSON API. Also adding the proper $fillable property to the models ofc.

Note to @emsici remember you're saving an array, so you have to cast the attribute as array in your model public $casts = ['certifications_id' => 'array']

marktopper commented 7 years ago

Great job reproducing this @Frondor. 👍

denbatte commented 7 years ago

Have the same issue on a multiple_select (sponsors). These sponsors have a name, a website and a logo (image).

image

image

Frondor commented 7 years ago

@denbatte can you paste your json details in sponsors field?

denbatte commented 7 years ago

This is the JSON in the sponsors BREAD field.

{
    "relationship": {
        "key": "id",
        "label": "name"
    }
}

Also the model:

class Stage extends Model
{
    public function sponsors(){
        return $this->belongsToMany(Sponsor::class, "stages_sponsors");
    }
}
Frondor commented 7 years ago

Sorry, and the model for that BREAD as well... if you're using one. It would probably be at app\Stage.php. I think there is something with eloquent's $fillable property.

@denbatte https://laravel-voyager.slack.com/

denbatte commented 7 years ago

Sorry, has been a while since I was active on GitHub. Updated my last post with code sample. Isn't here a more convenient channel to communicate?

denbatte commented 7 years ago

@Frondor do I need an invite for the slack channel?

denbatte commented 7 years ago

Ok, did some more testing. I created a oneToOne relation of a group and a stage and that works perfectly. Also, I made the sponsors field in the stages table nullable and this made the last error go away. There is a different error now though.

image

Having a look at the relationToLink method at the moment.

denbatte commented 7 years ago

@Frondor @marktopper I have found the issue. There is a mistake in the relationToLink method. It uses an array to get the label values in the code but actually, at the moment its a collection.

https://github.com/the-control-group/voyager/blob/master/src/Http/Controllers/Traits/BreadRelationshipParser.php

                if (!is_object($item[$field])) {
                    $item[$field] = $relation[$relationData->label]; // HERE IS MISTAKE
                } else {
                    $tmp = $item[$field];
                    $item[$field] = $tmp;
                }

I will try to create a PR if I find some time to fix it.

Frondor commented 7 years ago

I don't think that's the main problem. This is not only about relationships, since this same error happened with $timestamps fields... But that can be another bug. At the moment, it's working fine in my v0.11 branch

emsici commented 7 years ago

Do we need an invite to that Slack channel, or what?

denbatte commented 7 years ago

/offtopic @emsici there is a link with a slack invite website on the Voyager homepage ;) Just found out.

marktopper commented 7 years ago

/offtopic Get Slack Invite

fanzypantz commented 7 years ago

I seem to get the same issue, but who knows if this is just due to my part. Image of error Here is my parent and pivot table:

public function up()
    {
        Schema::create('orders', function (Blueprint $table) {
            $table->increments('id');
            $table->integer('user_id');
            $table->text('comment')->nullable();
            $table->integer('price');
            $table->boolean('sent');
            $table->boolean('paid');
            $table->string('products');
            $table->timestamps();
        });

        Schema::create('order_product', function (Blueprint $table) {
            $table->primary(array('order_id', 'product_id'));
            $table->integer('order_id')->unsigned();
            $table->foreign('order_id')->references('id')->on('orders')->onDelete('cascade');
            $table->integer('product_id')->unsigned();
            $table->foreign('product_id')->references('id')->on('products');
            $table->integer('amount');
            $table->timestamps();
        });
    }

I had to use composite keys to actually get to the "add item" page if not I got this error(snippet below) which suggest there is either something I did wrong or an error in the way the parts get selected due to not properly formatting the SQL select. I could have left the pivot table as normal and looked into the code, but a composite key at least got me further ahead.

SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'id' in field list is ambiguous (SQL: select id from products inner join order_product on products.id = order_product.product_id where order_product.order_id is null) (View: D:\WindowsFolders\Documents\PHP\Voyager-test\vendor\tcg\voyager\resources\views\bread\edit-add.blade.php)

Here is the child table:

public function up()
    {
        Schema::create('products', function (Blueprint $table) {
            $table->increments('id');
            $table->string('title')->nullable();
            $table->text('description');
            $table->string('image');
            $table->integer('price');
            $table->string('slug')->unique();
            $table->timestamps();
        });
    }

Here is the Model function:

public function products()
    {
        return $this->belongsToMany(Product::class, 'order_product');
    }

Here is just a few dummy products. Image of products They don't seem to show up properly when I select them in the products menu. Image of add page The "Order" BREAD: image of bread

I'm no real programmer yet and can't really help fix anything without great struggle, but I have a question about the many to many relationship. Do you guys plan to expand on it in the future? Because from what it seems like, it's a little restrictive. How would I change some extra data in the pivot table like the "amount" field I added into my pivot table. Because then I could easily define the amount of products of this type is related to the order.

Django admin have a very nice relationship system where you simply leave your models "as is" with no extra field to store data. And just add the relationship in an admin config. Then the admin page simply add the necessary inputs like in the picture below and you could add many children at the same time. The children wouldn't have to be premade. It might be too much to ask for a django like relationship system. Image of django admin

abacram commented 7 years ago

I had a similar issue when listing models in a many to many relationship. In my case, I unchecked the "browse" checkbox in the relationship (products in your case) for the bread configuration and it works fine.

Hope it helps!

hzung commented 7 years ago

@abacram: It's worked great! Thanks a lot, bro.

adriangordon1231 commented 7 years ago

What I had to do to get this to work is define the relationship in two function.

In your case, you would have to implement belongsToMany in certificationsId() AS WELL AS a certifications_id() function (the latter of which seems to be required in order for the model to save without throwing a 'Call to undefined method ' error).

fletch3555 commented 7 years ago

@adriangordon1231, that's a workaround, not a fix. But it hints at the core issue being that we must've missed something when changing from type_id() to typeId()

adriangordon1231 commented 7 years ago

@fletch3555 Never said it was a fix. All I did outline what I did to make it work.

handiwijoyo commented 6 years ago

Close this due to old version of Voyager. Please open new issue if this haven't been fixed.

github-actions[bot] commented 4 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. If you have further questions please ask in our Slack group.