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

FIX Apply urldecode to querystring vars #134

Conversation

emteknetnz
Copy link
Member

Fix for #133

I'm unsure about this approach because while it means that the space character can now be properly used in a filter value, the plus character can no longer be. It does seem pretty likely that space would be a far more used character than plus though

The developer who creates the custom report is required to put something like the following in their report for space to work (whether or not we merge this PR). Again, it has the same limitation that the space character can be used as part of a filter but not the plus character.

    public function sourceRecords($params = null)
    {
        array_walk_recursive($params, function(&$value) {
            $value = urldecode($value);
        });
        return MyModel::get()->filter($params);
    }

Sample code that can be used to test this PR:

MyModel.php

<?php

use SilverStripe\ORM\DataObject;

class MyModel extends DataObject
{
    private static $db = [
        'MyField' => 'Varchar'
    ];

    public function requireDefaultRecords()
    {
        parent::requireDefaultRecords();
        if (self::get()->count() > 0) {
            return;
        }
        $values = ['X', 'Y', 'A B', 'A+B'];
        foreach ($values as $value) {
            $record = self::create();
            $record->MyField = $value;
            $record->write();
        }
    }
}

MyReport.php

<?php

use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\TextField;
use SilverStripe\Reports\Report;

class MyReport extends Report
{
    public function title()
    {
        return 'My report';
    }

    public function sourceRecords($params = null)
    {
        array_walk_recursive($params, function(&$value) {
            $value = urldecode($value);
        });
        return MyModel::get()->filter($params);
    }

    public function columns()
    {
        return ['MyField' => 'MyField'];
    }

    public function parameterFields()
    {
        return new FieldList(
            new TextField('MyField', 'MyField')
        );
    }
}

@emteknetnz emteknetnz force-pushed the pulls/4.7/urldecode-params branch from 108cb52 to 7207c6b Compare January 11, 2021 23:40
Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

You're not addressing the real problem here. The issue is the front end encodes A B as A%2BB when it should be encoded as A%20B. The problem is in ReportAdmin.js

$.path.addSearchParams and $.param seem to be using different approaches to encode/decode URL params.

I think you can bypass this problem by flattening your serialized params array into an object map like this:

/**
 * File: ReportAdmin.js
 */
(function($) {
  $.entwine('ss', function($){
	  $('.ReportAdmin .cms-edit-form').entwine({
		  onsubmit: function(e) {
        var url = $.path.parseUrl(document.location.href).hrefNoSearch,
          params = this.find(':input[name^=filters]').serializeArray();
        params = $.grep(params, function(param) {return (param.value);}); // filter out empty

        params = params.reduce(function (accmulator, item) {
          accmulator[item.name] = item.value;
          return accmulator
        }, {});

        if(params) url = $.path.addSearchParams(url, params);
        $('.cms-container').loadPanel(url);
        return false;
		  }
	  });
  });
})(jQuery);

@emteknetnz emteknetnz closed this Feb 3, 2021
@maxime-rainville maxime-rainville deleted the pulls/4.7/urldecode-params branch February 3, 2021 08:36
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 this pull request may close these issues.

2 participants