scottmac / opengraph

Helper class for accessing the OpenGraph Protocol
463 stars 151 forks source link

Various enhancements #5

Open bashofmann opened 12 years ago

bashofmann commented 12 years ago

the only thing left to do would be:

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

ramsey commented 11 years ago

+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 commented 11 years ago

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 commented 11 years ago

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

bashofmann commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

Any ETA on when this PR might be merged?