@sslavic opened this Pull Request on June 23rd 2014 Contributor

This patch adds option to configure custom request content processing logic which gets called before request content is sent to tracker.

@sslavic commented on June 23rd 2014 Contributor

Tried to fix [1]st build failure with upgrade of qunit, but that didn't work out well, build failed again [2] and now with even more errors. Wish I could run the build locally before making more changes, but development environment setup for piwik project is not trivial.

[1] https://travis-ci.org/piwik/piwik/jobs/28220740
[2] https://travis-ci.org/piwik/piwik/jobs/28225867

@mattab commented on June 24th 2014 Member

Thanks for the pull request.

Feedback:

  • Running the tracker tests should be quite straightforward, what problem did you experience? to run the Javascript tests go to: localhost/piwik/tests/javascript/
  • The test failed because the code was not JSLint. Error was: Test failed in module externals: 'JSLint' Error: JSLint. Go to: http://jslint.com/ and there you can test that piwik.js code is lint.
  • to make sure that in the future we do not break this new feature, please add a new test case similar to this one that shows this new method works and allows you to modify request
@sslavic commented on June 24th 2014 Contributor

jslint issues fixed, unit tests provided. Build is successful, only run on one environment failed because of environment issue as a dependency could not be installed (for more details see https://travis-ci.org/piwik/piwik/builds/28309234 )

Now I see I need another related configuration option - possibility to configure POST request Content-Type header value. Currently it's hardcoded to 'application/x-www-form-urlencoded; charset=UTF-8'

@sslavic commented on June 24th 2014 Contributor

Again, environment issues, this time in two build steps, failed the entire build (see https://travis-ci.org/piwik/piwik/builds/28321164 ) but Javascript tests build step and 11 other steps ran well.

@mattab commented on June 24th 2014 Member

I see I need another related configuration option - possibility to configure POST request Content-Type header value

could you tell me the use case where you want to change the content-type?

@mattab commented on June 24th 2014 Member

Again, environment issues,

Yes you're right all these build failures are random and not caused by your change. We have this issue regularly and we will try to keep minimising the random build failure issue.

The tests you added are nicely done!

@sslavic commented on June 25th 2014 Contributor

With new method one can change request content not to be query parameters string, but a JSON or XML, or something else, and in these cases previously hardcoded form content type header value is not appropriate. So, since content can be customized, IMO content type header should be customizable as well so one can change it to match actual content; having one option without other would be incomplete.

@sslavic commented on June 25th 2014 Contributor

I've made default request method and request content type value DRY, while keeping setters safe. Build is all green.

@mattab commented on June 25th 2014 Member
This Pull Request was closed on June 25th 2014
Powered by GitHub Issue Mirror