[4.1][Feature] Relationship Field

[pxpm]

Hey there people! 👍

So @tabacitu before the number of changed files drives you way from this PR, I just want to state that the majority of those changes (all fields) was just to add a stack where the on-the-fly dialog pushes it's styles/scripts. 😈

So... relationship field.

This field can be a select2/select2_multiple/select2_from_ajax/select2_from_ajax_multiple (you don't need to specify nothing of this, we get it from relation type/column type/dev forced types 💃 )

$this->crud->addField([
           'name' => 'articles',
           'entity' => 'articles',
           'type' => 'relationship',
           'relation_type' => 'BelongsToMany',
           'attribute' => 'title',
           'pivot' => true,
//this part is not mandatory for field to work, only if you need to allow the creation
//of the related entities on the fly, in that case you also need to use the `OnTheFlyOperation`
           'on_the_fly' => [ 
               'create' => true,
               'entity_route' => 'article' //if different than entity
           ]
           ]);

By default this would setup a select2_multiple kind of field, you could specify 'ajax' => true to make it a select2_from_ajax_multiple.

When merging: #2310 and #2308 this field could simply become:

$this->crud->addField('entity_related_in_model') only need to setup the ajax entities if you are about to use it as ajax field. In that case would be: $this->crud->addField(['name' => 'entity_related_in_model','ajax' => true'])

If you use more ajax fields than regular I also setup a config where you could set relationship fields ajax ones by default.

Like we discussed @tabacitu it's a 500 lines of code field, but it's everything inside that file, so it should be easier to fix things as we go. (I really hope there is not tons of things to fix!)

WARN:

Please guys, it would help alot if you could test this in some project but don't use this as is in production. Any bug you found report it here, it's too much select this and that, relationship here and there and I might miss some use cases 😢

Wish the best to all you guys,
Pedro

[request-info]

Hi there!

Could you please provide us with more info about this? Looks like you skipped the title/body.

Thank you!

--
Justin Case
The Backpack Robot

[vonsogt]

I got this error:
Method Backpack\CRUD\app\Library\CrudPanel\CrudPanel::setRequest does not exist.

Solution:

• I just fixed by just copy the changed line only

[pxpm]

@vonsogt In this branch the setRequest() was removed from crud panel. The constructor of crud panel changed to:

public function __construct()
    {
        $this->request = \Request::instance();

        if ($this->getCurrentOperation()) {
            $this->setOperation($this->getCurrentOperation());
        }
    }

[vonsogt]

and the route is not defined too:
Route [entity-on-the-fly-create] not defined.

should I add more routes in custom.php? @pxpm

Should be fixed (in this case the on-the-fly create => false):

• and the relationship should have placeholder to avoid first pick id from the entity
  I tried to use placeholder but it's not working
• When I tried to edit, the selected entity can be select again from select2, etc
• when I tried to edit & save, will get this error
  
• When I tried to create new, it will only input created_at and updated_at
  SQLSTATE[HY000]: General error: 1364 Field 'personal_id' doesn't have a default value (SQL: insert intoorders (updated_at, created_at) values (2020-01-24 09:45:42, 2020-01-24 09:45:42))

I'm using this:

$this->crud->addField([
	'label' => 'Debitur',
	'type' => 'relationship',
	'name' => 'personal_id',
	'entity' => 'personal',
	'relation_type' => 'BelongsTo',
	'attribute' => 'name',
	'pivot' => true,
	'on_the_fly' => [
		'create' => false,
		'entity_route' => 'personal'
	]
]);

[pxpm]

Hello @vonsogt

I really appreciate you take the time to test and give feedback on this. Thank you very much for that.

Indeed, this is a work in progress and probably (for sure) has some bugs.

After your review I realized i focused on one side of the relationship field (the creation on the fly) and not so much on the relationship itself (This should be a good field even without using on the fly operation)

So, if you are willing to give it a second chance here is what to expect from relationship field:

[x] The syntax to create related fields should be simplified

• You can now add a relationship field like: $this->crud->addField(['name' => 'name_of_the_relation_in_model','type' => 'relationship']);

Extra keys: (Apart from the select2/select2_multiple keys that you can override)
'ajax' => true, // force the select to become ajax (either multiple or single)

[x] Allow the creation of relations «on the fly» but also play well as a standalone.

• Now you can use the relationship field as a standalone field, you just add the OnTheFlyOperation if you need it.

---

I also fixed some bugs on option selection. In your scenario i would test:

$this->crud->addField(['name' => 'personal', 'type' => 'relationship'])

I am still pushing commits to this branch, so make sure you are running my latest commits :)

Best,
Pedro

[tabacitu]

@pxpm I've taken the liberty of merging master into this branch and fixing the merge conflicts. So that we have a better understanding of the actual differences between this branch and master. That's the only change I made - and now it's 43 changed files instead of 56. Still a lot 😄 but better.

The next time you work on this, do a pull beforehand. If you have problems pushing your new work, feel free to create a new branch & PR that points to this branch, and we'll figure it out.

Let me know when this is ready to review.
Cheers!

[tabacitu]

@pxpm - kudos to you. This looks like A MONSTER 👹 of a PR. I can tell a lot of work has been put into this, and I really appreciate it. You the man! 🎆 🍾 🎈 🎉

Like you'd expect from a huge PR like this, I think there are quite a few things we can improve. It really helps to have a second pair of eyes on big features like this. When reading my feedback please understand I'm just trying to make this as good as possible, to be able to merge it and maintain it for years to come.

This first review includes "easy fixes" and a bunch of questions about "how does this work" to be able to give better feedback next time 😄 In this first review I expect we'll be able to:

• move files around;
• rename files and methods;
• talk about how some things work, so that we can find alternative solutions for some problems;

Thanks again! I'm really excited about having this feature 😄

PS. It looks like the unit tests are failing.

[tabacitu]

Hmm I can't find getRelationWithFieldName() anywhere. Not on master and not in this branch. Are your sure you've included this method too?

Also, if what it does is to return the relation using the field name (because the field name and the relation name should be the same), I would rename it to getRelationFromFieldName() (from, not with). If the method doesn't use fields inside it (like I think it won't), and it can also be used for Columns or such, I'd go with an even more general name, like getRelation(), getRelationData() or getRelationOnModel() whatever's closer to the truth, depending on what it returns.

Sidenote: Did you keep in mind that $field["name"] can be an arrray in Backpack 4.0? Not for the "relationship" field, I guess, but just saying - no idea if it applies here in any way.

[pxpm]

Agree with you in renaming to getRelationFromFieldName(). - fixed

The field name can be the relation name articles or the column name article_id - i created the functionality, but i have mixed feelings about having this here. It's more code to maintain, and well if it's a relationship field you should provide the direct relation name. I am willing to remove this feature if you share the same concerns as i do.

Do you see why would people use array as field name for a relationship field? Is it intended for example:

$this->crud->addField(['name' => ['relation1','relation2'],'type' => 'relationship']);

//instead of:

$this->crud->addField(['name' => 'relation1','type' => 'relationship']);
$this->crud->addField(['name' => 'relation2','type' => 'relationship']);

Adds a field for each relation ?

[tabacitu]

I don't see people using name arrays for the relationship field, no. I don't think that's worth supporting. Don't know why I mentioned that earlier, if we don't support it - we don't support it 😄 nothing we should do about it.

---

Hmm... you're right - nice catch with being able to put both category and category_id for the name, and be able to infer it's a relationship field. BUT. Wouldn't that interfere with the saving process? As far as I know, when saving:

• for 1-n relationship, the name has to be category_id (column in db)
• for n-n relationship, the name should be categories (relationship name) and pivot has to be true;

The way I understand this change, this would make it possible for 1-n relations to put both category_id and category for name - which would make it consistent between relationships, which is GREAT. But only if the saving process still works 😄

[vonsogt]

I really need this feature thanks @pxpm & @tabacitu, I would test this soon

[tabacitu]

OMG @pxpm ! This already looks SO MUCH better. I'm so glad you've found a solution that doesn't require us to add that blade stack to all fields - I can rest in peace now 😄

Jokes aside, thanks for putting up with me, this already looks much cleaner. I've replied with a new round of suggestions (and the last one, I think) - this time much easier.

After this round, I'll just dig deeper inside the relationship.blade.php file and try to test as many use cases as possible, to make sure we're bug free.

Great job!

[tabacitu]

Looking good @pxpm ! Can't wait to finally have this - it's become A LOT easier to understand and work with, but it's still a very complex feature.

For now, I think we only need to fix the few minor issues I've raised. And fix the failing unit tests. Then I can merge this WIP branch into 4.1 so we can test them together. Like we talked though, let's push fixes to this feature here, on this branch, please.

Cheers!

[pxpm]

I am fixing this @tabacitu .

[scrutinizer-notifier]

A new inspection was created.

[tabacitu]

Merged into 4.1. For bug fixing let's use #2548 instead.