Switch to feed-io for parsing

[SMillerDev]

This is a work in progress fixing #258

EDIT: All tests should be fine and my current imports work nicely on my server

[janvonde]

@SMillerDev any progress on this? I can't install this News app via the Nextcloud App Store because it is not listed there and a manual installation failes because during running the make command it can not find the picofeed stuff 😃

[SMillerDev]

I thought I pushed the fixes for this. Ah well, it needs a review before it can go anywhere anyway.

[BernhardPosselt]

@janvonde download the latest archive from here https://apps.nextcloud.com/apps/news/releases?platform=13#13

[SMillerDev]

@nextcloud/news any opinions?

[danopz]

Just did a code review.
In general it would be very nice if you could define variable/response types to enable IDEs to follow your code (thats on my list anyway for the whole app).
Also I would like to use a coding standard, personally I am using Psr-2/Psr-12, so far haven't read the NC coding style. What do you think?

[danopz]

Maybe we could add some service for stuff like this? I don't like it to be on an entity which should just be a representation of data from the database

[SMillerDev]

That seems like something for a broader cleanup.

[SMillerDev]

I'll add the php 7 types. As for coding standards, I did a run for that a while ago. The official one is zend if I recall correctly but I wouldn't have an issue with any PSR as long as we document it somewhere.

[SMillerDev]

The official one seems to be pear, are you okay with that @danopz ? Can you make a pull request adding the standard to the README in any case?

https://docs.nextcloud.com/server/14/developer_manual/general/codingguidelines.html#php

EDIT: @nextcloud/news could we enable https://styleci.io/ for that maybe?

[SMillerDev]

@nicola88 @danopz @Grotax CI is green, what do you think?

[Grotax]

Nice, I will test it tomorrow and maybe take a brief look on the code.

[nicola88]

I will test it too and give a look at the code.

[danopz]

I will try to get some spare time to do another review.
Btw it would have been better to not force but this branch, but to do another commit, because we could just see the change. If you want to sqash those commits we should do it afterwards.

[Grotax]

Adding a new feed causes an error for me:
https://gist.github.com/Grotax/ba51ec4a658afa07b4c77876dc35e496

[SMillerDev]

Thanks for the comment @danopz, force of habit I guess. Made sure to pay attention this time. @Grotax I have to say I didn't do a real world test with that. I think the last commit should at least make it clearer why it failed. Could you try again with that?

[Grotax]

PHP Fatal error:  Declaration of OCA\News\Fetcher\FeedFetcher::fetch(string $url, $getFavicon = true, $lastModified = NULL, $user = NULL, $password = NULL): array must be compatible with OCA\News\Fetcher\IFeedFetcher::fetch($url, $getFavicon = true, $lastModified = NULL, $user = NULL, $password = NULL): array in /var/www/nextcloud/apps/news/lib/Fetcher/FeedFetcher.php on line 28

[Grotax]

Still broken but no errors in the nextcloud.log
Errors are more verbose now (had dropped the tables between testing got a lot of text)
But I didn't get an error for adding a Feed.
Current Situation:

1. Install app, clean tables
2. add feed
   2.1 the feed I used in the last test now fails in the ui but the error makes no sense (see image)
   2.2 other feeds are added but only the last added feed stays in the left feed overview
3. wait or run cron manually
4. no feed update for the feed

Some observations:

Content Security Policy: Die Einstellungen der Seite haben das Laden einer Ressource auf eval blockiert ("script-src"). angular.min.js:203

In browser console not sure if thats an issue (maybe caused by my setup localhost=webserver)

Feeds are in DB but no items.

[SMillerDev]

@Grotax that turned out to be a pretty difficult one as it doesn't publish update times. I hope I fixed it now, can you try again?

[Grotax]

First test:
Subscribe to every recommended feed in the news app ✔️
Second test:
Import opml from current stable, with my feeds ❌
If I import my opml file and the server encounters an error the import will stop.
There is a notification to check the server logs but it disappears in milliseconds.
https://gist.github.com/Grotax/e1315a0d81c4ffae54f85bc91882bc29

EDIT: it seems like all the other feeds were imported correctly but the ui behavior is not cool but that's another issue

[Grotax]

Apart from the import error i couldn't find any issues 👍

[SMillerDev]

I had the import issue too on something unrelated. Could you check if this also happens on stable?

[Grotax]

I tried the same Import on 13.03, works fine.
Subscribing with feedIO-version leads to 403

<outline title="Segfault" text="Segfault" type="rss" xmlUrl="http://segfault.linuxmint.com/feed/" htmlUrl="http://segfault.linuxmint.com/"/>

The blog is actually already moved to another website but you can still get the feed.

[SMillerDev]

If I subscribe without a OPML that feed throws a CSRF exception. Could you check with the new location to see if it's in the blog or in the server config?

[Grotax]

The new official location works fine. I think in that case we are done. I don't see any reason not to merge maybe @nicola88 want's to take another look?

[nicola88]

All good for me, too. I tried to import/edit/delete a few feeds and then I imported an OPML file with all the 70+ feeds I usually follow and the only error I received was a request timeout for a feed which apparently no longer exists.

[SMillerDev]

@danopz @MorrisJobke @skjnldsv could this be merged then?

[MorrisJobke]

@danopz @MorrisJobke @skjnldsv could this be merged then?

Fine by me. I would give this over to you. @nicola88 or @Grotax could review this PR and then every contributor can merge this PR. This allows you to independently work on the news app.

[Grotax]

"At least 1 approving review is required by reviewers with write access."
I don't have write access, so my review doesn't change GitHubs opinion about merging ;)

[MorrisJobke]

@Grotax I invited you :)