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

API /reader/api/0/stream/items/contents #1774

Merged
merged 10 commits into from Feb 8, 2018
Merged

Conversation

Alkarex
Copy link
Member

@Alkarex Alkarex commented Jan 25, 2018

@Alkarex Alkarex added this to the 1.10.0 milestone Jan 25, 2018
@Alkarex Alkarex added the Work in progress 🚧 Wait before merging label Jan 25, 2018
@aledeg aledeg added the API 🤝 API for other clients label Jan 28, 2018
@Frenzie
Copy link
Member

Frenzie commented Jan 30, 2018

So they use a slightly incompatible Google Reader API?

@Alkarex
Copy link
Member Author

Alkarex commented Jan 30, 2018

@Frenzie More some features that I did not implement because I had not seen anybody using them :-)

For FeedMe.
This token is not used by e.g. The Old Reader API.
There is the Authorization header anyway.
TODO: Check security consequences
FeedMe uses GET for some parameters typically given by POST
@Alkarex Alkarex modified the milestones: 1.11.0, 1.10.0 Feb 5, 2018
@Alkarex Alkarex removed the Work in progress 🚧 Wait before merging label Feb 5, 2018
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there such a thing as API docs?

@@ -216,9 +216,13 @@ function token($conf) {
function checkToken($conf, $token) {
//http://code.google.com/p/google-reader-api/wiki/ActionToken
$user = Minz_Session::param('currentUser', '_');
if ($user !== '_' && $token == '') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== or ===?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict for the first comparison, loose for the second (match empty string and 0 in particular)

@@ -476,73 +528,37 @@ function streamContents($path, $include_target, $start_time, $count, $order, $ex
break;
}

if (!empty($continuation)) {
if ($continuation != '') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're fine if it's FALSE or 0? Also, is $continuation trimmed anywhere or could you end up with something empty-adjacent like a space?

Also, like above, != or !==?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, loose comparison there. The test should reject empty string, false, 0, null, etc. and only keep substantial strings.
But good point with sanitising those parameters. Will do so.

Copy link
Member

@Frenzie Frenzie Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so != something falsy (loose) basically means exactly the same thing as empty()? I know that PHP plays a bit fast and loose with things like 1 and 0 vs true and false, but I didn't realize empty strings were included in being falsey. I thought any string was truthy. Apparently the string '0' is also falsey.

Confusingly, on that last note, 0.0 (float) is falsey and '0.0' (string) is truthy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed. empty() is mostly relevant when working with potentially non-existing entries such as in a array, and it is similar to doing !isset($t[1]) || $t[1] == ''. In the case where we have a variable and not an index, there is little point of using empty() and it used to be slower (I am not sure whether there is any difference nowadays).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find empty() slightly clearer in the sense that I don't necessarily think of an empty string as falsey while I do think of it as empty, but that's just my problem I suppose. :-P

Python is much more consistent in counting empty arrays, empty strings, etc. as falsey than the likes of JavaScript or PHP. But in Ruby, for example, empty strings are truthy, and in a sense they are in Lua too. In POSIX and related shells you also have to be well aware to use checks like -z (unset or empty) and -n (set and non-empty).

Incidentally, can we get this kind of fun out of PHP?

bla = '0'; if (bla && bla == false) console.log('/sigh')
/sigh

It's truthy (non-empty string) but at the same time '0' == false. Perhaps even more confusingly, you can get the same behavior out of new Boolean(false) (the existence of any object is inherently truthy in JS, but its value == false).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun, but no PHP. Well, at least I got the last one right after the hint. I didn't really dare think not a number at first because that just seemed too logical. xD

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, I have one for you. Guess the output :-P

<?php
echo ctype_digit(1234) ? 'true' : 'false', "\n";
echo ctype_digit(123) ? 'true' : 'false', "\n";
echo ctype_digit('123') ? 'true' : 'false', "\n";

Copy link
Member

@Frenzie Frenzie Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intuitive guess would be false false true but I'm guessing there's a catch. Now to check… ;-)

Edit: hm, true false true. Well then…

If an integer between -128 and 255 inclusive is provided, it is interpreted as the ASCII value of a single character (negative values have 256 added in order to allow characters in the Extended ASCII range). Any other integer is interpreted as a string containing the decimal digits of the integer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close enough: true false true. :-)

If an integer between -128 and 255 inclusive is provided, it is interpreted as the ASCII value of a single character (negative values have 256 added in order to allow characters in the Extended ASCII range). Any other integer is interpreted as a string containing the decimal digits of the integer.

@Alkarex
Copy link
Member Author

Alkarex commented Feb 7, 2018

A new version of FeedMe will be released when we release FreshRSS 1.10.0 with this patch.

@Alkarex
Copy link
Member Author

Alkarex commented Feb 8, 2018

Preview:
screenshot_20180208-140434

@Alkarex Alkarex merged commit 79f8b44 into FreshRSS:dev Feb 8, 2018
@Alkarex Alkarex deleted the API_FeedMe branch February 8, 2018 19:11
@Alkarex
Copy link
Member Author

Alkarex commented Feb 8, 2018

@MaluNoPeleke and others: The /dev branch of FreshRSS (soon 1.10.0) and FeedMe 3.5.3+ (currently available as beta in the Play Store) should work fine together. Tests welcome.

@Alkarex
Copy link
Member Author

Alkarex commented Feb 8, 2018

#328

This was referenced Feb 8, 2018
@bramboomen
Copy link

Hi, I just set-up a fresh install of FreshRSS dev and FeedMe 3.5.4 and encountered an issue.
The list of articles does not synchronize. As far as I can tell the connection is working because starred items do synchronize and FeedMe has the complete list of my subscriptions but no unstarred, read or unread articles.
I did have some issues with setting up the API, in particular with the PATH_INFO variable in nginx, but currently both api checks pass and the EasyRSS application is working without problem.
I would try some manual API calls to further debug the problem, but I have no idea where to begin in that endeavour...

@Alkarex
Copy link
Member Author

Alkarex commented Feb 18, 2018

@blankoworld Thanks for the test.
Please make a detailed log by enabling the two following lines

FreshRSS/p/api/greader.php

Lines 740 to 743 in c79b8fb

//Minz_Log::debug('----------------------------------------------------------------', API_LOG);
//Minz_Log::debug(debugInfo(), API_LOG);

and either set 'environment' => 'development', in ./data/config.php or change the lines to Minz_Log::warning().
It will be in ./data/users/_/log_api.txt

@bramboomen
Copy link

@Alkarex
Happy to, let me know if I can do anything else!
log_api.txt

@Alkarex
Copy link
Member Author

Alkarex commented Feb 18, 2018

@bramboomen Could you please ensure that you have a number of unread articles before trying a first synchronisation?
If that still does not work, would it be possible for you to make me a test account on your FreshRSS instance?

@bramboomen
Copy link

@Alkarex
There were both unread and read articles but they only show up if I star them.
I will send you an email with the information for the test-account.

@Alkarex
Copy link
Member Author

Alkarex commented Feb 18, 2018

Thanks for the test account. It seems to work fine for me :-)
Can you give a try to the test account you have just made for me?

@Alkarex
Copy link
Member Author

Alkarex commented Feb 18, 2018

Ah, in FeedMe, under Settings / Synchronisation Mode, pick "All". (I am not sure of the exact wording of the English version)

@bramboomen
Copy link

@Alkarex
Synchronizing 'all' instead of 'per feed' did the trick, thank you!

@Alwaysin
Copy link
Contributor

Had the same problem!
I guess a lot of people might conclude "this is not working" and go somewhere else.

Anyway, this reader is a nice addition, I'll use it a couple days to see if it fits my needs :)

@Alkarex
Copy link
Member Author

Alkarex commented Mar 1, 2018

The next version of FeedMe will have the proper synchronisation setting by default.

@fcoiffie
Copy link

fcoiffie commented Mar 3, 2018

Regarding the FeedMe configuration, it's not very easy to guess that the Host is: http://adresse_serveur/p/api/greader.php

@Alkarex
Copy link
Member Author

Alkarex commented Mar 3, 2018

@fcoiffie Indeed, therefore:

Please note that is is better to expose only the ./p/ directory of FreshRSS (and not its parent), to have a public URL like https://freshrss.example/ instead of https://freshrss.example/p/.

javerous pushed a commit to javerous/FreshRSS that referenced this pull request Jan 20, 2020
* API /reader/api/0/stream/items/contents

For FeedMe

* Fix continuation

* Continuation in stream/items/ids

* Fix multiple continuations

* Allow empty POST tokens

For FeedMe.
This token is not used by e.g. The Old Reader API.
There is the Authorization header anyway.
TODO: Check security consequences

* API compatibility FeedMe: add/remove feed

FeedMe uses GET for some parameters typically given by POST

* A bit of sanitization

* Links to FeedMe

* API favicons more robust when base_url is not set

* Changelog FeedMe
mdemoss pushed a commit to mdemoss/FreshRSS that referenced this pull request Mar 25, 2021
* API /reader/api/0/stream/items/contents

For FeedMe

* Fix continuation

* Continuation in stream/items/ids

* Fix multiple continuations

* Allow empty POST tokens

For FeedMe.
This token is not used by e.g. The Old Reader API.
There is the Authorization header anyway.
TODO: Check security consequences

* API compatibility FeedMe: add/remove feed

FeedMe uses GET for some parameters typically given by POST

* A bit of sanitization

* Links to FeedMe

* API favicons more robust when base_url is not set

* Changelog FeedMe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API 🤝 API for other clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants