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

3.4.0 - regression - "Don't load relation for each column and allow dot notation in field name for index table" #2674

Closed
jandol5 opened this issue Oct 16, 2024 · 1 comment · Fixed by #2675

Comments

@jandol5
Copy link

jandol5 commented Oct 16, 2024

Description

The 3.4.0 has introduced "Don't load relation for each column and allow dot notation in field name for index table" (#2603) which breaks previous behavior.

Steps to reproduce

Add additional relation index table columns in controller, eg

protected function additionalIndexTableColumns(): TableColumns
{
    return parent::additionalIndexTableColumns()
        ->add(Relation::make()->relation('measure')->field('title')->title('Naslov'))
        ->add(Relation::make()->relation('measure')->field('category.title')->title('Naziv kategorije'));
}

Expected result

Index table should have additional two columns:

  • "Naslov" containing the title of the "measure" relation instance
  • "Naziv kategorije" containing the title of category category relation of "measure" relation instance

Actual result

Using the dot notation in filed name throws Illuminate\Database\QueryException stating SQLSTATE[42S22]: Column not found: 1054 Unknown column 'category.title' in 'field list' (Connection: mysql, SQL: select category.titlefrommeasureswheremeasures.deleted_at is null).
When I remove Relation::make()->relation('measure')->field('category.title')->title('Naziv kategorije') and use only Relation::make()->relation('measure')->field('title')->title('Naslov'), the index table renders the additional 'Naslov' column but the data is wrong: all titles of all of the relation model instances are displayed.

Possible fix

The PR #2603 changes $relation type from \Illuminate\Database\Eloquent\Collection to \App\Models\[model] as seen in src/Services/Listings/Columns/Relation.php. If I replace the new $relation = $model->getRelation($this->relation); with previous $relation = $model->{$this->relation}()->get(); I get the expected result.

@Tofandel is this an innocent regression or did I miss something?

Versions

Twill version: 3.4.0
Laravel version: 11.28.0
PHP version: 8.3.4
Database engine: MySQL 5.7.24

@Tofandel
Copy link
Contributor

Tofandel commented Oct 16, 2024

Could you please share your models

$model->loadMissing($this->relation);
$relation = $model->getRelation($this->relation);

And

$relation = $model->{$this->relation}()->get();

Should be functionnally equivalent (except one uses the cache and one runs a query everytime)

Is measure a one to one relation?

Edit: just checked and yeah in one to one ->get still returns a collection but not getRelation

In the PR I mentionned that "however the Relation column doesn't actually use the loaded relation (and doesn't work with one to one relation)"

So I did try originally with a one to one and didn't get it to work, so I guess that's why I didn't catch this regression

Tofandel added a commit to Tofandel/twill that referenced this issue Oct 16, 2024
In the case of one to one getRelation returns a model and the pluck call invokes a query instead of invoking pluck on the collection

So instead wrap it to make sure we always get a collection
@Tofandel Tofandel mentioned this issue Oct 16, 2024
@ifox ifox closed this as completed in bb21695 Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants