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

Wrong docs regarding JSONP callback parameter #2566

Closed
jesobreira opened this issue Oct 11, 2019 · 7 comments

Comments

@jesobreira
Copy link

@jesobreira jesobreira commented Oct 11, 2019

Sorry for removing the bug report template, but I believe this is too simple.

The docs state that, for using JSONP, it's needed to add a "jsonp" parameter with the callback function name.

JSONP support: add a JSONP callback with a jsonp parameter (eg query string ?jsonp=my_callback)

However the source code looks for a variable named callback instead.

if( isset( $_REQUEST['callback'] ) )
	$return['callback'] = $_REQUEST['callback'];

Indeed, requests that follow the docs (with ?jsonp=) do not work (the response comes without the callback name), whereas if we use ?callback= instead, it works.

@jesobreira jesobreira added the bug label Oct 11, 2019
@dgw dgw changed the title Wrong dogs regarding JSONP callback parameter Wrong docs regarding JSONP callback parameter Oct 11, 2019
@dgw dgw added bug docs good first issue and removed bug labels Oct 11, 2019
@dgw

This comment has been minimized.

Copy link
Member

@dgw dgw commented Oct 11, 2019

Assuming the others agree that callback is a fine name for the parameter (I think it is better than jsonp), this would make a great Hacktoberfest issue for someone wanting to get involved in the YOURLS project! Possibly you, if you're interested. 😸

(Leaving the parameter name as it is in the code and updating the docs to match, instead of the other way 'round, also avoids breaking any existing uses of this functionality.)

@LeoColomb

This comment has been minimized.

Copy link
Member

@LeoColomb LeoColomb commented Oct 11, 2019

LGTM
Great suggestion for the Hacktoberfest! 👍

@bluemorbo

This comment has been minimized.

Copy link
Contributor

@bluemorbo bluemorbo commented Oct 12, 2019

This might like sound a daft question to a GitHub newbie, but where do the docs live? I can't find a reference to the code in this repo. Thanks in advance 😄

@ozh

This comment has been minimized.

Copy link
Member

@ozh ozh commented Oct 12, 2019

Hold on, might not be just this. We have a jsonp callback param here when used with a bookmarklet.

Maybe it's a case of consistency between API and bookmarklet

@ozh

This comment has been minimized.

Copy link
Member

@ozh ozh commented Oct 12, 2019

@bluemorbo in the wiki for some more advanced stuff, yourls.org for the basics

@bluemorbo

This comment has been minimized.

Copy link
Contributor

@bluemorbo bluemorbo commented Oct 13, 2019

Since it appears it's more likely an inconsistency between jsonp and callback, we've a few different options:

  1. Update docs to reflect the inconsistency
  2. Add support for both parameters to the API so the docs are correct while still remaining backwards compatible
  3. Update API to only reference a jsonp parameter, with the understanding that existing uses of the JSON-P functionality would break.

I'm inclined to 2 myself, but happy to implement whichever changes are preferred. Let me know :)

@ozh

This comment has been minimized.

Copy link
Member

@ozh ozh commented Oct 13, 2019

Same for me, option 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.