cloudyr / limer

A LimeSurvey R Client
MIT License
67 stars 37 forks source link

Fixes #65: stop double parsing json #66

Closed mfavetti closed 8 months ago

Jan-E commented 8 months ago

Does not this break getting the session key in any Limesurvey before 6.3.5? See this post in the forum: https://forums.limesurvey.org/forum/installation-a-update-issues/127953-remotecontrol-doesn-t-work-after-update-to-5-3?start=12#252450 And the LS bugreport: https://bugs.limesurvey.org/view.php?id=19258

mfavetti commented 8 months ago

no it won't break backwards compat

previous behavior was to parse the response as json

new behavior is to parse the response in text format as json

so in old version with weird content type, response is parsed as json, content type not in httr list of handled types, so not parsed already in new version with correct content type, automatic json parsing is not used, again response as text is parsed as json

problem was with correct content type, response was already parsed json, then was parsed again

cbgdev commented 8 months ago

@andrewheiss Could you please merge this PR? It's been tested for backward compatibility and it's working.

Shnoulle commented 8 months ago

@mfavetti if this update is done because this commit : https://bugs.limesurvey.org/view.php?id=19214

There are an issue in https://bugs.limesurvey.org/view.php?id=19214 :

  1. MAJOR version when you make incompatible API changes
  2. MINOR version when you add functionality in a backward compatible manner
  3. PATCH version when you make backward compatible bug fixes

https://semver.org/ here need a MAJOR version update

Jan-E commented 8 months ago

Limer needs to be updated anyway. The PR is backwards compatible with older LS versions, so this PR can be merged now. There is no valid reason to postpone merging it. If Limesurvey wants to revert https://bugs.limesurvey.org/view.php?id=19214 is up to the LS devs.

andrewheiss commented 8 months ago

Great, thanks!