FIX: Do not blindly pass input values to GridField_FormAction URL's #1250

Merged
merged 5 commits into from May 8, 2013

Projects

None yet

4 participants

@wilr
SilverStripe Ltd. member

The length of input fields can very quickly exceed the max URI length resulting in 414 errors when printing or exporting results.

To access the input values for a specific GridField action, encapsulate this in your own Entwine instance.

@chillu
SilverStripe Ltd. member

Have you checked this still works with common GridField modules like sortablegridfield or this inline editing module? Does it still work with ModelAdmin searches and inline searches on GridField? I'm sure there was a reason we did this, also the approach obviously falls down for the reasons you mentioned. Sounds like it would be an API change though, at least for people extending GridField behaviour.

wilr added some commits Mar 4, 2013
@wilr wilr Fix alignment of selects within GridField filters 2e36e45
@wilr wilr Add no results warning to GridField print view. dda6fa6
@wilr wilr FIX GridField export and print actions should preserve state. 36d3303
@wilr wilr Docblock and coding conventions for GridField related classes. 1ddd1dd
@wilr wilr FIX: Do not blindly pass input values to GridField_FormAction URL's
The length of input fields can very quickly exceed the max URI length resulting in 414 errors when printing or exporting results.

To access the input values for a specific GridField action, encapsulate this in your own Entwine instance.
1853fc8
@wilr
SilverStripe Ltd. member

Will double check those modules tonight but this behavior was for buttons (such as print and export) rather than drag and drop. But yes, for those modules which added button components it would break. I can remark it as API.

ModelAdmin is working fine (was using it for testing). Also noticed that the print and export stuff is untested at this stage. Will investigate Behat tests for this but this will take me a couple days. At the moment this bug means exporting results is pretty much useless so keen to get it in if we do a beta3

@sminnee
SilverStripe Ltd. member

Did you get anywhere with this, @wilr?

@wilr
SilverStripe Ltd. member

@sminnee checked the modules and they perform fine. Haven't got around to playing with Behat, but it would be good to merge this before waiting for those tests.

@wilr
SilverStripe Ltd. member

If you want to check this pull, test the export or print function from userforms or any other modeladmin interface with more than a couple submissions.

Thanks

@wilr
SilverStripe Ltd. member

Any chance we can slip this in before the next beta? Does seriously limit the usefulness of exporting data :)

@chillu chillu merged commit a1216b5 into silverstripe:3.1 May 8, 2013

1 check passed

Details default The Travis build passed
@wilr wilr referenced this pull request in silverstripe/silverstripe-userforms May 10, 2013
Closed

'Export to CSV' or 'Print' NOT working #123

@jflearn jflearn commented on the diff Jun 5, 2013
forms/gridfield/GridFieldExportButton.php
@@ -119,7 +119,7 @@ public function generateExportFileData($gridField) {
$fileData .= "\n";
}
- $items = $gridField->getList();
+ $items = $gridField->getManipulatedList();
@jflearn
jflearn Jun 5, 2013

Hi! I seem to be running into a problem exporting recently and I've traced it back to this change. The problem I am having is that now the export button only exports the first page of a paginated list. Is that the intention? I would think that would not be what the user is expecting. Probably the same can be said for printing.

Obviously, I can get around this by rolling my own variant of the export button, but something doesn't seem right about this to me, so I though I'd comment.

@wilr
wilr Jun 6, 2013

@jflearn can you raise a new issue. I think the fix would be to setLimit to unlimited. We want to keep the manipulate list for the filter but the limit is the only think we want to remove.

@wilr wilr deleted the unknown repository branch Jun 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment