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

Various enhancements #5

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

Conversation

bashofmann
Copy link

  • travis ci support
  • composer support
  • psr-0 compatibility
  • encoding fixes
  • fallback to title and description meta tag if no og tags are available
  • exchanged file_get_content to buzz browser, so that content fetching can be mocked in tests
  • some code cleanup (doc comments, code formatting)
  • tests against fixtures instead of doing http requests

the only thing left to do would be:

setting up the project at travis ci and submitting it to packagist

- travis ci support
- composer support
- psr-0 compatibility
- encoding fixes
- fallback to title and description meta tag if no og tags are available
- exchanged file_get_content to buzz browser, so that content fetching can be mocked in tests
- some code cleanup (doc comments, code formating)
@ramsey
Copy link

ramsey commented Dec 4, 2012

+1

I considered doing the same thing, but since this PR is here, I'll probably just fork and pull in this PR to my own fork, so that I can use these changes.

@MitchellMcKenna
Copy link
Contributor

I love the composer support, psr-0 code cleanup, and travis ci support, but I'm not such a fan of the opengraph library suddenly having an external dependency on Buzz browser. file_get_contents() has now been replaced with a cURL call, see #8 #4. I know you partially added Buzz for mockabiliity, but I like that this library is currently so simple with no external dependancies.

@scottmac would you be more likely to merge in these changes if @bashofmann removed the dependency on Buzz and just left it as the cURL call?

@scottmac
Copy link
Owner

Yeah and I prefer PSR-0 without namespaces since I want to keep 5.2 support

@bashofmann
Copy link
Author

Sure I can rebase it on your changes and remove the namespaces. I'd still extract the actual curl request into it's own class though, so that you can mock the call and test the library. Thoughts?

@MitchellMcKenna
Copy link
Contributor

@bashofmann Just my opinion here, but seems like a shame to create another class just for the cURL call. What if the cURL call was broke out into it's own function so it could be stubbed? (Let me know if this doesn't work, my PHPUnit experience is limited.)

static public function fetch($URI) {
  $response = self::query($URI);
  return self::parse($response);
}

// This cURL call can now be stubbed.
static public function query($URI) {
  //cURL call
}

// Changing this to public, maybe user wants to query their own way and pass it in
static public function parse($HTML) {
  //same as before
}

@Argonalyst
Copy link

Well, nothing related to the code itself, but I was wondering.. If we are using the Open Graph to retrieve meta property tags, that the website owner is giving to us, to be used for social purposes.. The og:tags should be considered a Public Domain? I mean, we have a license to use it as a Semantic Web? og:image as an example, we have a license to use this thumbnail image?

Because if we modify the code to retrieve the Title or the Description that is not inside the og:tags, it could be a copyright infringement depending the way we use that data.

In my opinion, when a website owner decide to implement the Open Graph to its code, the content inside the tags should be free to be used with no restrictions...

@bashofmann
Copy link
Author

@MitchellMcKenna also fine, we could just overwrite these methods in the tests. In the end it boils down to a matter of taste, I like extracting this into it's own class more since it is a clearer separation of concerns (making the request vs parsing the response), since it's his library I guess @scottmac should decide which way he likes better.

@argonath I'm not a lawyer but I guess copyright wise this would fall under the same rules as content snippets in search engines, though maybe you would have to respect a robots file if present.

@scottmac
Copy link
Owner

I'm indifferent here, not a huge fan of adding extra dependencies but cool to do it if its optional. Go with whatever is easier to test, mocking an object is probably easier to do.

@faceleg
Copy link

faceleg commented Apr 18, 2013

Any ETA on when this PR might be merged?

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.

6 participants