Laravel-Backpack / community-forum

A workspace to discuss improvement and feature ideas, before they're actually implemented.
28 stars 0 forks source link

[4.2/5.0][Proposal] "column" extra attribute on fields #35

Open JoepdeJong opened 4 years ago

JoepdeJong commented 4 years ago

Following up Laravel-Backpack/CRUD#1905 lasts https://github.com/Laravel-Backpack/CRUD/issues/1905#issuecomment-504869509. There is still no simple way to have multiple fields with the same name. I my current project I need an pick-up and a delivery address, both are 1-1 related to the order and both have almost the same fields.

I think the solution of using a dot notation proposed by @tabacitu does not work for nested/1-1 related fields. When using a dot in the field name, the fields array turns into a multi dimensional array, which results in the following exception:

Facade\Ignition\Exceptions\ViewException Undefined index: type (View: /***/vendor/backpack/crud/src/resources/views/crud/inc/show_tabbed_fields.blade.php)

We could resolve this by making 'show_fields.blade.php' recursive, but then all the "isColumnNullable" checks in the field's blade fail, since the field name with dot does not exist as a column in the DB table. I propose adding a attribute called 'column_name' to the crud, which overrides the 'name' attribute on places where the name is used as a reference to the column. In this way we can still use the 'name' attribute to directly set the name of the field in HTML.

welcome[bot] commented 4 years ago

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 mediums:

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

tabacitu commented 4 years ago

Hi @JoepdeJong ,

I think the relationship field type will provide a solution to this. It will provide you with a select, but also let you create the connected entry right then and there, if there isn't one you can select:

Category:
_____________________ ▾ [Create]

The Create button will open a modal where you can create that connected entity, with AJAX. This would solve it in a different way, because you would only need one field to store the relationship in the main form. Not all fields, for all attributes.

I propose adding a attribute called 'column_name' to the crud, which overrides the 'name' attribute on places where the name is used as a reference to the column. In this way we can still use the 'name' attribute to directly set the name of the field in HTML.

Hmm this sounds interesting. Could you please explain a little more how this would work? So:

Cheers!

pxpm commented 4 years ago

Just as a side note, isColumnNullable will not throw any error if the field does not exists in database. It will return true instead. There was an error with Exception not defined. I pushed a PR to fix it.

try {
            //check if column exists in database
            $conn->getDoctrineColumn($table, $column_name);

            return ! $conn->getDoctrineColumn($table, $column_name)->getNotnull();
        } catch (\Exception $e) {
            return true;
        }
JoepdeJong commented 4 years ago

A: @pxpm

Just as a side note, isColumnNullable will not throw any error if the field does not exists in database.

I think it does. The exception is not thrown by Backpack, but by Doctrine/DBAL itself. In the following example I used a field name with a certain prefix (pickUpAddress) without dot notation. (When I use a dot notation I get the "type" exception)

Schermafbeelding 2020-02-10 om 16 09 58


B: @tabacitu The main functionality of this dashboard will be entering pickUp and delivery addresses (both around 15 unique db columns. i.e. street, zip, note, ....). Perhaps this will be extended in the future by multiple stops, instead of just these two. So we can't afford to use modals for this main functionality. Similarly, we can't use any mutators, so we have chosen to make the addresses simultaneously editable (in the form of multiple backpack tabs). To make this work we need the possibility to define fields with the same name but a unique prefix.

Using prefixes without dot notation ("pickUpAddress_customer_id" and "deliveryAddress_customer_id") throws the exception stated above (A), since this column name is not defined.

Using prefixes with dot notation (pickUpAddress), as suggested as a not tested solution, does not work. Since the fields array becomes multi-dimensional (array_add in the Fields trait turns dot notation into multidimensional arrays), the item 'pickUpAddress' contains the corresponding fields with as key the field name (after the dot) and as value the field array. The "type" field is thus not defined in the item 'pickUpAddress', so the exception stated in the initial issue will be thrown.

To resolve this problem, I suggest making the "show_fields" file recursive:

{{-- Show the inputs --}}
@foreach ($fields as $field)
    <!-- load the view from type and view_namespace attribute if set -->
    @if(!isset($field['name']))
        @include('crud::inc.show_fields', ['fields' =>  $field])
    @else
        @php
            $fieldsViewNamespace = $field['view_namespace'] ?? 'crud::fields';
        @endphp

        @include($fieldsViewNamespace.'.'.$field['type'], ['field' => $field])
    @endif
@endforeach

But then again exception A is thrown. Since the field "pickUpAddress.customer_id" does not exist as a column in the database. Indeed, the field name is "customer_id". Now we are back at my initial suggestion: making the field name in the crud fields which are used to select the database column, overwritable. This is not needed for fields like 'text' and 'number', but it is for fields like 'select2':

@if ($entity_model::isColumnNullable($field['name']))

Can be changed into: @if ($entity_model::isColumnNullable(isset($field['column_name'])?$field['column_name']:$field['name']))

Or into: @if ($entity_model::isColumnNullable(@end(explode('.',$field['name']))))

Then it is possible to show multiple fields with the same column name in the crud. But the tabs wont work anymore, since 'pickUpAddress' does not have a field 'tab'.

Even though using multi-dimensional arrays for prefixed field names seems intuitively the way to go for me, this can probably be resolved by using another separator which is uncommon in db column names (| or ,).

{{-- Show the inputs --}}
@foreach ($fields as $field)
    <!-- load the view from type and view_namespace attribute if set -->
    @php
        $field['name'] = str_replace('|', '.', $field['name']);
        $fieldsViewNamespace = $field['view_namespace'] ?? 'crud::fields';
    @endphp

    @include($fieldsViewNamespace.'.'.$field['type'], ['field' => $field])
@endforeach

Another solution could be adding a 'name_prefix' field to the crud, which will be prepended to the "fieldKey" in the Fields trait. Then we do not need to change name passed to "isColumnNullable", but the prefix must be added to all the other places where the name is used.

pxpm commented 4 years ago

What is the output of php artisan backpack:version @JoepdeJong ?

I am afraid I did not fully understand what you are trying to achieve, but from my understanding you want to show fields for two different tables in database ?

If that is the case you could do something like

$this->crud->addField([      
            'label'         => 'one select field)',
            'type'          => 'select2',
            'name'          => 'main_address', 
            'entity'        => 'address', 
            'attribute'     => 'title', 
        ]);
$this->crud->addField([      
            'label'         => 'one select field)',
            'type'          => 'select2',
            'name'          => 'second_address', 
            'entity'        => 'address', 
            'attribute'     => 'title', 
        ]);

Bare with me, it should NOT (it might, but shoudn't ) throw any exception. ìsColumnNullable returns true if column does not exists in database. (that's a recent change, that's why I am asking for your bp version).

After you might want to intercept your request in controller to perform your own saving process: check here the docs

Best, Pedro

JoepdeJong commented 4 years ago

@pxpm

What is the output of php artisan backpack:version?

### PHP VERSION:
PHP 7.4.2 (cli) (built: Jan 22 2020 06:30:58) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.2, Copyright (c), by Zend Technologies

### LARAVEL VERSION:
laravel/framework                     v6.14.0   The Laravel Framework.

### BACKPACK VERSION:
backpack/crud                         4.0.38    Quickly build an admin interfaces using Laravel 6, CoreUI, Boostrap 4 and jQuery.
backpack/generators                   2.0.6     Generate files for laravel projects

Using 'main_address' trows an exception with my version. I indeed want to show (multiple) fields from instances of the same table. When the exception is gone I think the solution you proposed works for the Create page. But on update the fields won't be filled when I do not create any mutators.

pxpm commented 4 years ago

I get it @JoepdeJong , thanks for info.

Is there any problem creating the mutators ? It's a one time stop, do it once and you good to go.

I am afraid atm that is the best option for what you want to achieve. Or I did not fully understand your problem.

Also I am sorry i made a mistake, isCollumnNullable WILL NOT throw error in future versions of backpack. You can grab the code from the developer branch:

https://github.com/Laravel-Backpack/CRUD/blob/35aabc3eb33d9d78357704124bbe867683d3837d/src/app/Models/Traits/CrudTrait.php#L111

So, you might be able to overwite the update function and add your custom value to your fields we will only try to get value if the value key is not present in the field array.

https://github.com/Laravel-Backpack/CRUD/blob/addb73e578ba48817539df75fb2fec6f18a56a60/src/app/Library/CrudPanel/Traits/Update.php#L53

Is this enough for you to work with ?

Best, Pedro

tabacitu commented 4 years ago

@JoepdeJong ,

The main functionality of this dashboard will be entering pickUp and delivery addresses (both around 15 unique db columns. i.e. street, zip, note, ....). Perhaps this will be extended in the future by multiple stops, instead of just these two. So we can't afford to use modals for this main functionality. Similarly, we can't use any mutators, so we have chosen to make the addresses simultaneously editable (in the form of multiple backpack tabs). To make this work we need the possibility to define fields with the same name but a unique prefix.

Using prefixes without dot notation ("pickUpAddress_customer_id" and "deliveryAddress_customer_id") throws the exception stated above (A), since this column name is not defined.

Now I think I understand your use case, thank you 😄

Solution 1. Instead of using the select2 field, you could use select2_from_array. This field doesn't check if the db column is nullable or not.

Solution 2. If your only problem is with the non_nullable feature, you could just overwrite the field and remove that one thing in your project.

I think Solution 1 would be the easiest for you. You could then have:

public function addAddressFields($prefix = "")
{
   $this->crud->addField($prefix.'address_line_1');
   $this->crud->addField($prefix.'address_line_2');
   // ..
}

public function create() {
   $this->addAddressFields();
   $this->addAddressFields("interm_");
   $this->addAddressFields("dropoff_");
   // ...
}

Let me know what you think. Cheers!

tabacitu commented 4 years ago

Now, about the actual proposal in this thread - adding a column attribute on fields.

At first glance, yes, it would help in this particular use case. It would provide a little more flexibility. But it might also make it MUCH more difficult to understand how fields work.

Right now:

After this proposed change:

I guess it wouldn't be too difficult to understand. What do you think @pxpm ?

There are a few more difficult things to keep in mind, though:

Note 1: For security, before saving, Backpack strips the request of any inputs that were not added using Backpack fields. So that users can't inject inputs in the form using Developer Tools. That saving bit would need changing too - they would need to use the column, not name.

Note 2: What happens when someone changes the name of a field with modifyField()? Would we change the column too?

@JoepdeJong forgive me if I'm a bit skeptical, and please expect the decision process to take a while. Keep in mind that our opinion comes from a different place than yours, because:

DanielKimmich commented 4 years ago

I think a possible solution would be: In the model define in the property "$appends" the field to use

protected $appends = [ '**name_first**', ];
    Public function getNameFirstAttribute()
    {    return $this->data2;   }

In MyCrudController I define the field as follows

        $this->crud->addField([
            'name'  => **'name_first**',
            'label' => 'First Name',            
            'type'  => 'text',
            'store_in' => '**data2**', // [No optional]
            ]);

So in the compactFakeFields function I must verify if the $field['name'] is in the "appends" array of the model, if so I replace it with $field ['store_in']).

As we know Backppack cannot use mutators, in this way we would be doing something similar.

Example:

      update(["name_first" => "Gladys", "age" => "20"] );

By

      update(["**data2**" => "Gladys", "age" => "20"] );