opengraph
opengraph copied to clipboard
scottmac
Various enhancements
- 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
+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.
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?
Yeah and I prefer PSR-0 without namespaces since I want to keep 5.2 support
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?
@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
}
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...
@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.
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.
