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

Convert comma separated param values to arrays if needed #12796

Merged
merged 1 commit into from May 2, 2018

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Apr 30, 2018

fixes #12627

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Apr 30, 2018
@sgiehl sgiehl added this to the 3.5.0 milestone Apr 30, 2018
if (is_array($defaultValue)) {
$value = Piwik::getArrayFromApiParameter($value);
}
return $value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this affect other parameters as well? Eg, the 'label' parameter or a parameter like it might have commas in it but might not be an array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it shouldn't, as those parameters should not have an array as default value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, nvm then.

@diosmosis
Copy link
Member

LGTM. Do you think it would be worthwhile to mention this change in the changelog? That plugins that add overridable array ViewDataTable properties can now accept comma separated lists from query parameters? Although I originally remember this being supported by columns_to_display, so maybe it's more of a regression.

@sgiehl
Copy link
Member Author

sgiehl commented Apr 30, 2018

columns could be given as comma separated list. Not sure if that applied for columns_to_display as well. I'm not sure if we should mention it in the changelog... @mattab any opinion?

@diosmosis
Copy link
Member

Right, maybe I'm thinking about columns

@mattab mattab merged commit e0ba540 into 3.x-dev May 2, 2018
@mattab mattab deleted the convertparams branch May 2, 2018 08:04
@mattab
Copy link
Member

mattab commented May 2, 2018

I reckon it's not needed to mention it in this case

InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fatal error: Cannot unset string offsets in JqplotDataGenerator
3 participants