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

add unit testing and using Guzzle #12

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

peter279k
Copy link

Change log

  • add unit testing.
  • use Guzzle to request the RSS feed.
  • fix the if statement logic.

@dg
Copy link
Owner

dg commented Oct 17, 2016

Thank you for PR. What is the benefit to use the Guzzle instead of cURL?

@peter279k
Copy link
Author

  1. Using Guzzle to request the url easily and make the code readability.
  2. We don't need to add the if-statement to check the cURL extension because the Guzzle will check the extension automatically.
  3. Guzzle is an abstraction layer for HTTP transport which happens to use cURL where available.
  4. Guzzle simplifies things enormously, particularly when it comes to debugging.

@grunjol
Copy link

grunjol commented Mar 6, 2017

I am +1 on this PR.

Ideally you should pass a PSR-7 compatible implementation and use it.

As @perter279k said Guzzle is a very good choice and I can add to the list the killer feature: middlewares

Interesting middlewares for this project can be

  • Cache: Instead of your disk cache you can delegate it to a cache middleware (and use disk, redis, memcache, etc)
  • Retry MIddleware: sometimes your feed gets overloaded and you get a temporary 5XX error, with a retry middleware you can configure to retry X times with a configurable interval.

@dg
Copy link
Owner

dg commented Mar 6, 2017

@grunjol I have no experience with this. Can you please give an example code, how can be used middleware for cache or retry used?

@grunjol
Copy link

grunjol commented Mar 6, 2017

Today I had to implement it for a project I am working on, so I rushed it and took @peter279k master (which this PR is based on) and forked the repo.
I changed a little bit the interface (moved auth user/pass as options for the request) so, this will break the current API, but the good thing is I was able to create a working Cache (and a cloudflare busting mechanism)

Check it out https://github.com/grunjol/rss-php, I put some examples in the README file including a volatile (memory) cache, you can use any other cache backend (https://github.com/Kevinrob/guzzle-cache-middleware)

@dg
Copy link
Owner

dg commented Mar 6, 2017

Thanks!

@m1guelpf
Copy link

Any updates on this?

@peter279k
Copy link
Author

I don't receive any request changes for while.

I still wait for the response :).

@m1guelpf
Copy link

@dg Can you merge this in?

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

Successfully merging this pull request may close these issues.

4 participants