[Bug] Dash symbol in <span></span> tag when using model_function for columns

[mariovillani]

Bug report

What I did

Upgrade from 4.x to 5.x version

What I expected to happen

No impact on columns that use "model_function" type

What happened

After upgrading from Backpack 4.x to Backpack 5.x, a - appeared in HTML when using "model_function" type in column. Sample code (first Backpack controller, then model function):

/* Backpack controller code */
$this->crud->addColumn([ // n-n relationship (with pivot table)
    'name' => "company_id",
    'label' => "Company", // Table column heading
    'type' => "model_function",
    'function_name' => 'linkCompany' // the method in your Model
]);

/* Function in Company model */
public function linkCompany() {
    echo '<a href="'.backpack_url('tabcompany').'/'.$this->company_id.'/edit">'.$this->tab_company->company_name.'</a>';
    return;
}

<!-- Output in HTML -->
<td><a href="https://mysite.com/admin/tabcompany/14/edit">Company name</a>
<span>-</span> <!-- this is not supposed to be here! -->
</td>

What I've already tried to fix it

Trying to change to directly return (as the documentation states in https://backpackforlaravel.com/docs/5.x/crud-columns#model_function-1):

/* Function in Company model */
public function linkCompany() {
    return '<a href="'.backpack_url('tabcompany').'/'.$this->company_id.'/edit">'.$this->tab_company->company_name.'</a>';
}

but the tag <a> is always wrapped in a <span>, so it is not shown correctly.

Is it a bug in the latest version of Backpack?

Yes

Backpack, Laravel, PHP, DB version

Laravel 9.x (latest), Backpack 5.x (latest)

[pxpm]

Hello @MarioVillani thanks for the report, I have some questions here:

Can you please do a dd($this->crud->columns() after defining that column and post it here ?

Are you overriding the model_function column in your resources folder and you need to update the file to match the package one ?

In Backpack v5 we introduced the wrapper for this use-cases, https://backpackforlaravel.com/docs/5.x/crud-columns#wrap-column-text-in-an-html-element

Wouldn't that fix your use case, and avoid running the model function for each row ?

Let me know, Cheers

[mariovillani]

Here is the output:

^ array:3 [▼
  "id" => array:8 [▼
    "name" => "id"
    "label" => "ID"
    "type" => "text"
    "key" => "id"
    "priority" => 0
    "tableColumn" => true
    "orderable" => true
    "searchLogic" => true
  ]
  "datetime" => array:9 [▼
    "name" => "datetime"
    "label" => "Data"
    "type" => "datetime"
    "format" => "l HH:mm:ss"
    "key" => "datetime"
    "priority" => 1
    "tableColumn" => true
    "orderable" => true
    "searchLogic" => true
  ]
  "company_id" => array:9 [▼
    "name" => "company_id"
    "label" => "Company"
    "type" => "model_function"
    "function_name" => "linkCompany"
    "key" => "company_id"
    "priority" => 2
    "tableColumn" => true
    "orderable" => true
    "searchLogic" => true
  ]
]

I'm not overriding the model_function column...

[pxpm]

@MarioVillani I may have gone in the wrong direction here.

When you say it does not display correctly, it displays the "html" instead of the proper link, is that the case?

If it is, please have a look at Step 23 from the upgrade guide: https://backpackforlaravel.com/docs/5.x/upgrade-guide#security-1

You probably need to add escape => false to that column.

Let me know if that was the issue.

Cheers

[mariovillani]

Hello, i've tried adding escaped=> false as you've said, without touching the model function, but it does not change the output at all (does not change if i specify escaped => true neither).

So i've changed my model function by directly returning the HTML block and removing the echo function call:

public function linkCompany() {
    return '<a href="'.backpack_url('tabcompany').'/'.$this->company_id.'/edit">'.$this->tab_company->company_name.'</a>';
}

In this case, if i specify escaped => true i can see the anchor element (not rendered in HTML), if i specify escaped => false i do not get any output at all, except the <span></span> i've mentioned before.

What could the problem be?

[pxpm]

I think I get it now. Thanks @MarioVillani for your inputs.

This was introduced in https://github.com/Laravel-Backpack/CRUD/pull/3936

The model_function should never rely on column['value'], because the value will never come from the db entry, but from the model after running the function.

I think you hit an edge case, that is usually a model function, you give it a random column name, in your case you are giving it a "real column name", so $value = $column['value'] ?? get.it.from.model.function()

This shouldn't be happening or the model_function may not have the desired results everytime.

I will submit a fix for that. Thanks for the report.

About your implementation, If company_id is a belongsTo relation called company let's say, you should be defining your column like this:

[   
    'name'      => 'company.company_name',
    'wrapper'   => [
        'href' => function ($crud, $column, $entry, $related_key) {
            return backpack_url('tabcompany/'.$related_key.'/edit');
        },
    ],
],

Let me know, I will be submitting the PR to fix the model_function issue.

Cheers

[mariovillani]

I think that the way i've implemented my code was the suggested way for Backpack v.3.x or v.4.x. If i remember correctly i've took some examples from the documentation itself.

However, i'm glad to help! I'll wait for the fix, i prefer not impacting my code right now. Thanks! Let me know.

[pxpm]

Indeed, I aknowledge that's what was suggested to use at that time. But since you are updating to v5, you could take advantage of the improvements we did in the meanwhile. I was just suggesting a more optimized way of achieving the same end goal, it was not meant to criticize the way you implemented. Sorry if it looked like that 🙏

Using relation.attribute will make the relation to be eager loaded, reducing the queries, resource usage and page loading time. By using the wrapper functionality to generate the <a> will also improve the performance since you will not be executing model functions for each row.

There is more than one way to get to Rome, yours should work too as soon as we review and merge https://github.com/Laravel-Backpack/CRUD/pull/4668 that reverts the bad change we did 😞 Sorry for that! 🙏

Let me know if I can help you with something else.

Cheers

[mariovillani]

Understood! Eager loading that would surely be better. I'll edit my code then. Thanks!

[pxpm]

I will be closing this since the PR is just waiting review and should be merged anytime soon I hope.

Cheers @MarioVillani and thanks for the report 🙏