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

Support array for json_data posted in rest/json service #99

Merged
merged 1 commit into from Mar 27, 2020
Merged

Support array for json_data posted in rest/json service #99

merged 1 commit into from Mar 27, 2020

Conversation

Ousret
Copy link
Contributor

@Ousret Ousret commented Nov 4, 2019

When using x-www-form-urlencoded we could leverage the possibility to send array instead of raw json as PHP support it out of the box.

This change/bugfix is not BC-Break.

I do not know if this is a bug or not, when reading current documentation, it is not clearly stated that json_data should be a string.

auth_user=my_user&auth_pwd=my_password&json_data%5Boperation%5D=core%2Fget&json_data%5Bclass%5D=ServiceChange&json_data%5Bkey%5D=SELECT+ServiceChange+WHERE+id%3D1&json_data%5Boutput_fields%5D=request_state

instead of

auth_user=my_user&auth_pwd=my_password&json_data=%7B%22operation%22%3A+%22core%2Fget%22%2C+%22class%22%3A+%22ServiceChange%22%2C+%22key%22%3A+%22SELECT+ServiceChange+WHERE+id%3D1%22%2C+%22output_fields%22%3A+%22request_state%22%7D

It is more convenient to post json_data[operation]=core/get instead of json_data={"operation": "core/get"..

When using x-www-form-urlencoded we could leverage the possibility to send array instead of raw json as PHP support it.
@Hipska
Copy link
Contributor

Hipska commented Nov 4, 2019

Actually, JSON is a string by definition, but I like your suggestion.

@Ousret
Copy link
Contributor Author

Ousret commented Nov 4, 2019

@Hipska absolutely JSON is a string. But sending raw JSON through HTTP x-www-form-urlencoded is not a good idea for maintenance/readability. Or we could support application/json instead of x-www-form-urlencoded. That could work too.

@piRGoif
Copy link
Contributor

piRGoif commented Nov 6, 2019

Just a note for when this PR will be reviewed : there is a corresponding Combodo support UR, referenced R-023452

@Ousret
Copy link
Contributor Author

Ousret commented Nov 18, 2019

@Ousret Ousret changed the title 🐛 Support array for json_data posted in rest/json service Support array for json_data posted in rest/json service Dec 2, 2019
@Ousret

This comment has been minimized.

@piRGoif
Copy link
Contributor

piRGoif commented Jan 10, 2020

J'ai peut-être un projet interessant à partager avec la communauté iTop.

Please use english :)
And also, you should better communicate on this in the appropriate forum section : https://sourceforge.net/p/itop/discussion/third-party-extensions/

@rquetiez rquetiez added this to the 2.8.0 milestone Feb 7, 2020
@piRGoif
Copy link
Contributor

piRGoif commented Feb 7, 2020

This was submitted in Combodo ticket DB as N°2767, candidate for iTop 2.8.

@Ousret sorry it took us so much time to accept this PR... We were (and still are in a way) quite busy with the 2.7.0... We are aiming to integrate this in 2.8.0.

@piRGoif piRGoif removed their assignment Feb 7, 2020
@Hipska
Copy link
Contributor

Hipska commented Mar 18, 2020

If this would be implemented, can I suggest that the params would then be like this?

auth_user=my_user&auth_pwd=my_password&operation=core%2Fget&class=ServiceChange&key=SELECT+ServiceChange+WHERE+id%3D1&output_fields=request_state

@piRGoif piRGoif self-assigned this Mar 27, 2020
@piRGoif piRGoif added the question Further information is requested label Mar 27, 2020
@piRGoif
Copy link
Contributor

piRGoif commented Mar 27, 2020

Hello,
I just did a review, the code seems ok but I would prefer to test it with real world examples...

Here is below a PHP caller using current json_data as string. Can you send back a script that would generate the new parameter format ?

<?php

$curl = curl_init();


$aStdOptions = array('auth_user' => 'admin','auth_pwd' => 'admin');

// JSON form
$aJsonData = array('json_data' => '{
   "operation": "core/get",
   "class": "Person",
   "key": "SELECT Person", "limit": "10", "page": "1"
}');

	curl_setopt_array($curl, array(
	CURLOPT_URL => 'http://localhost/itop-dev/webservices/rest.php?version=1.4',
	CURLOPT_RETURNTRANSFER => true,
	CURLOPT_ENCODING => "",
	CURLOPT_MAXREDIRS => 10,
	CURLOPT_TIMEOUT => 0,
	CURLOPT_FOLLOWLOCATION => true,
	CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1,
	CURLOPT_CUSTOMREQUEST => "POST",
	CURLOPT_POSTFIELDS => array_merge($aStdOptions, $aJsonData),
	CURLOPT_HTTPHEADER => array(
		'Cookie: XDEBUG_SESSION=netbeans-ide',
	),
));

$response = curl_exec($curl);

curl_close($curl);
echo $response;

@Hipska
Copy link
Contributor

Hipska commented Mar 27, 2020

When implementing my suggestion, it would be:

$aJsonData = array(
   "operation" => "core/get",
   "class" => "Person",
   "key" => "SELECT Person",
   "limit" => 10,
   "page" => 1
);

When the initial suggestion is implemented, it would be:

$aJsonData = array(
    "json_data[operation]" => "core/get",
    "json_data[class]" => "Person",
    "json_data[key]" => "SELECT Person",
    "json_data[limit]" => 10,
    "json_data[page]" => 1
 );

@piRGoif
Copy link
Contributor

piRGoif commented Mar 27, 2020

Many thanks Thomas, I was able to test then :)

I like your solution... Should be doable with little modification... I'm merging this for now :)

@piRGoif piRGoif removed the question Further information is requested label Mar 27, 2020
@piRGoif piRGoif merged commit 79cfb95 into Combodo:develop Mar 27, 2020
@piRGoif
Copy link
Contributor

piRGoif commented Mar 27, 2020

When implementing my suggestion, it would be:

$aJsonData = array(
   "operation" => "core/get",
   "class" => "Person",
   "key" => "SELECT Person",
   "limit" => 10,
   "page" => 1
);

I implemented something to handle this, see #130. @Hipska and @Ousret I'd like to have your feedback in this new PR !

@piRGoif
Copy link
Contributor

piRGoif commented Mar 27, 2020

@Ousret I will add you to the contributors list in our README.md. How would you prefer to be seen ? With your real name, your github name ?

@Ousret
Copy link
Contributor Author

Ousret commented Mar 28, 2020

With my Github name, or both. 👍

piRGoif pushed a commit that referenced this pull request Mar 30, 2020
@piRGoif piRGoif modified the milestones: 2.8.0, 2.7.1 Jun 17, 2020
@piRGoif
Copy link
Contributor

piRGoif commented Jun 17, 2020

This was also merged to support/2.7 branch, and will be available in the future 2.7.1 version.

@Hipska
Copy link
Contributor

Hipska commented Jun 18, 2021

Doc has been updated. https://www.itophub.io/wiki/page?id=latest%3Aadvancedtopics%3Arest_json

@Ousret I don't see this reflected in the docs (Also not for 2.7)

@piRGoif
Copy link
Contributor

piRGoif commented Jun 18, 2021

Ousret comment was made in nov 2019...
What did you expect to see in the wiki page @Hipska ?

@Hipska
Copy link
Contributor

Hipska commented Jun 18, 2021

Okay, now you make me wonder what he meant. What exactly was updated in the doc? Since I would expect a line somewhere you could also use the array syntax as introduced by this PR..

@piRGoif
Copy link
Contributor

piRGoif commented Dec 31, 2021

Woops I forgot to answer about the documentation sorry !

I've just published this little extra paragraph : https://www.itophub.io/wiki/page?id=latest%3Aadvancedtopics%3Arest_json#parameters_in_a_php_array_form

@Hipska
Copy link
Contributor

Hipska commented Jan 10, 2022

Great, maybe also mention this on the 2.7 wiki pages for people staying on the LTS release?

@piRGoif
Copy link
Contributor

piRGoif commented Jan 17, 2022

Done : https://www.itophub.io/wiki/page?id=2_7_0%3Aadvancedtopics%3Arest_json#parameters_in_a_php_array_form

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants