Added option to configure custom request content processing logic #325
Conversation
…ore request content is sent to tracker
|
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 |
|
Thanks for the pull request. Feedback:
|
This reverts commit 53ff607.
…er and setter for custom data
|
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' |
|
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. |
| * @param string requestContentType; default is 'application/x-www-form-urlencoded; charset=UTF-8' | ||
| */ | ||
| setRequestContentType: function (requestContentType) { | ||
| configRequestContentType = requestContentType || 'application/x-www-form-urlencoded; charset=UTF-8'; |
There was a problem hiding this comment.
the default value is not needed here because it is set to default above (it's best to avoid duplication when possible)
There was a problem hiding this comment.
Yes, it's a bit redundant, deliberately, to be consistent with existing code, e.g. setRequestMethod. When you think of it, it's like a safe setter, if setter is called without providing value. To be DRY and keep setters safe, we could make in both cases default value to be a separate non mutable field.
could you tell me the use case where you want to change the content-type? |
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! |
|
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. |
|
I've made default request method and request content type value DRY, while keeping setters safe. Build is all green. |
Added option to configure custom request content processing logic fixes #5384
|
method was renamed in: a0c1fee and documentation updated in: matomo-org/developer-documentation@bc29074 |
This patch adds option to configure custom request content processing logic which gets called before request content is sent to tracker.