-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[WIP] added HeiseBridge #744
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice bridge, I like it a lot. 👍
Find below a few suggestions
bridges/HeiseBridge.php
Outdated
protected function parseItem($feedItem) { | ||
$item = parent::parseItem($feedItem); | ||
|
||
$article = getSimpleHTMLDOMCached($item['uri']) or returnServerError('Could not open article: ' . $url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think $url
works in this context, as that variable is never initialized. $item['uri']
should work
bridges/HeiseBridge.php
Outdated
} | ||
$article = $article->find('div.article-content', 0); | ||
|
||
$item['author'] = $author; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is no other place where $author
is used, you can assign the value directly (without variable).
Definitely yes. Currently the bridge throws an error on each request, which is not desirable. You can reduce the likelihood of it happening by adding a user limit for the number of articles to return (default should be a reasonable value like 3 or 5). The way this needs to be done is a bit awkward (because you need to know the inner workings of RSS-Bridge): 1) Introduce a parameter to specify the optional user limit 'limit' => array(
'name' => 'Limit',
'type' => 'number',
'required' => false,
'title' => 'Specify number of full articles to return',
'defaultValue' => 5
) Notice: The limit is not required, so you should handle situations where it isn't set (use default value). 2) Check if enough items have been added (only add titles for the rest) This is how your protected function parseItem($feedItem) {
$item = parent::parseItem($feedItem);
$limit = $this->getInput('limit') ?: 5;
if(count($this->items) >= $limit) {
return $item;
}
... Notice: If the limit is not set, the default limit (5) is used. You could use a constant to make sure the same limits are used in the parameters and the code. This is what I see for a limit of 1: |
Request: Support multi-page articles 😁 You can load the print preview instead of the regular article to get rid of ads (and load multi-page articles in one go!). This should work on all articles: Just add |
I rebased on master and made some necessary changes to receive contents (pagination and no-ads). This works for the most part. Instead of filtering the stuff that isn't needed, it now only takes elements that are of interest. There is of course a chance that something is still missing, but that can be added later anyway. |
The current bridge is now part of the repository. If you are still interested in adding support for reviews, please open a new PR. |
The bridge expands the RSS-feed of the german news site www.heise.de
Running this on my server throws a PHP memory exception. I was too lazy to change the memory limit so I just did it in the code. (Commented out in this PR). Is there anything I should do to try to catch this exception or something like that?
I did this for personal use and I'm in no way a PHP-expert. So, if you have suggestions on how to improve the code please leave a comment and I will try to implement it.
TODO: