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

Using allFields() method breaks export of relationship and computed fields #48

Closed
3 tasks done
duncmc opened this issue Apr 9, 2019 · 8 comments
Closed
3 tasks done

Comments

@duncmc
Copy link

duncmc commented Apr 9, 2019

Prerequisites

  • Able to reproduce the behaviour outside of your code, the problem is isolated to Laravel Excel.
  • Checked that your issue isn't already filed.
  • Checked if no PR was submitted that fixes this problem.

Versions

  • PHP version: 7.2.14
  • Laravel version: 5.8.10
  • Nova version: 2.0.1
  • Package version: 3.1.11 and 1.1.3

Description

Using allFields() method breaks export of relationship and computed fields.

Steps to Reproduce

Ensure that your Resource includes a field that is defined with a BelongsTo relationship…

BelongsTo::make('User'),

…or field that returns a computed value…

Text::make('Random number', function () {
    return str_random();
}),

Use the allFields() method on the action as described in the documentation…

public function actions(Request $request)
{
    return [
        (new DownloadExcel)->allFields(),
    ];
}

Expected behavior:

The exported file should contain a representation of the relationship similar to how it appears on the Nova Resource index page. Similarly the file should contain the output generated by closure defined on the computed field.

Actual behavior:

The related record is represented in the Excel file as a foreign key. Computed fields are omitted.

Additional Information

This is already a known issue as indicated by a common made on a previous issue.

Are you using ->allFields() perhaps?

Originally posted by @patrickbrouwers in #26 (comment)

Given that the dev team has been aware of this since at least November 2018 I assume it is difficult or impractical to fix. However it would be useful to have this “feature” documented on the "Customizing Exports” page of the documentation and anywhere else that may be relevant.

@patrickbrouwers
Copy link
Member

allFields is a bit of a wrong naming. What it actually does is getting all "attributes" from the Eloquent model and doesn't consult the Nova resource to see what kind of field it is.

I'll probably deprecate and replace this method in the next release by allAttributes to align with the actual functionality.

If you want to have an export that represents how the nova resource index page looks, you should just not using allFields at all and it should work out of the box.

@pelmered
Copy link

allFields is a bit of a wrong naming. What it actually does is getting all "attributes" from the Eloquent model and doesn't consult the Nova resource to see what kind of field it is.

I'll probably deprecate and replace this method in the next release by allAttributes to align with the actual functionality.

If you want to have an export that represents how the nova resource index page looks, you should just not using allFields at all and it should work out of the box.

What should I do if I want to include all fields including relationships and computed values?

@patrickbrouwers
Copy link
Member

@pelmered with "all fields" do you mean all fields that are defined as "index" fields on the nova resource? Than you don't have to do anything, those are used by default.

@pelmered
Copy link

@patrickbrouwers No, all fields that are defined on the resource no matter where they are displayed. In my case I would like to include all those fields except one.

I think this is the most common use case, because you probably want to avoid side scrolling to use the buttons on the row.

@patrickbrouwers
Copy link
Member

I think currently you would have to specify all fields with ->only() that you want in the export.
We might need to change allFields to do that instead. Feel free to PR that functionality. Happy to incorporate it as a solution in the package.

@ghost
Copy link

ghost commented Jul 2, 2019

+1 on the ability to have a true allFields functionality!

@xoco70
Copy link

xoco70 commented Jul 26, 2020

Same here. If using only(), when having a relationship with user_id, I can use user that will display the whole json object, but not user.email

@patrickbrouwers
Copy link
Member

PR was merged

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

No branches or pull requests

4 participants