Problem/Motivation

In ChartsPluginStyleChart::render(), if only one field is selected, $data_fields can be empty. This means that $data_field_key will be null on line 367. When that is used as an argument to ::processNumberValueFromField() on line 395, $field will be null leading to a fatal error when it is used as an argument to ::fieldProvidesArrayData() on line 751.

Proposed resolution

Return early if $data_fields is empty.

Remaining tasks

Determine if this is the best solution. Implement.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#3 charts-empty_data-3624137-3.patch899 bytesliam morland

Issue fork charts-3624137

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

liam morland created an issue. See original summary.

liam morland opened merge request !234

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Active » Needs review
StatusFileSize
new charts-empty_data-3624137-3.patch899 bytes

This patch is the contents of the merge request.

andileco’s picture

andileco commented
Status: Needs review » Needs work

Thank you! Can you investigate the failing test?

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

I think what is happening is that the test chart doesn't have any data. So, with this patch, it returns empty. Previously, it would give a blank chart, but still some chart properties are set. I'm not sure that is what it should do. Maybe the test should be changed. Alternatively, the fix in this issue could be changed so that it still returns the $chart array but avoids the error in another way.

andileco’s picture

andileco commented

Would you be able to share more about what you're trying to accomplish so that I can try to replicate in my own environment?

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

I had a View with only one field and the result was a whitescreen TypeError:

TypeError: Drupal\charts\Plugin\views\style\ChartsPluginStyleChart::fieldProvidesArrayData(): Argument #1 ($field_key) must be of type string, null given, called in /.../modules/contrib/charts/src/Plugin/views/style/ChartsPluginStyleChart.php on line 751 in Drupal\charts\Plugin\views\style\ChartsPluginStyleChart->fieldProvidesArrayData() (line 782 of modules/contrib/charts/src/Plugin/views/style/ChartsPluginStyleChart.php).

A chart needs more than one field but it shouldn't result in a whitescreen if it is setup that way.

andileco’s picture

andileco commented

@liam morland, I wasn't able to replicate your error. Here are the steps I followed:
I opened a view displaying a bar chart (I tried with both Highcharts and Chart.js). It used the title field for the label and two numeric fields as data series.
I removed the two data fields from the view and refreshed the preview. It showed a chart with no data, but nothing threw an error or showed up in the logs.
I opened up the chart settings and clicked apply. That gave me the expected validation error that says that at least one data field must be present.

Can you share your process and how it was different from this?

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

I'm not sure exactly. I was working on an existing site and one of the Views was giving the error message. That View had just one field and the message went away when I added another field. There could have been something else that is off about that View. In any case, it shouldn't give a whitescreen.

nikathone made their first commit to this issue’s fork.

nikathone’s picture

nikathone
Primary language Swahili
Location Ontario
commented
Status: Needs work » Needs review
liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

This looks good, though I think the change to ChartsTestLibrary.php probably belongs in its own issue.

andileco’s picture

andileco commented

@liam morland - I have updated the MR: this is my preferred solution. Does this work for you?

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs review » Reviewed & tested by the community

Yes

andileco’s picture

andileco commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.