-
Notifications
You must be signed in to change notification settings - Fork 38
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
Parse an image for src and alt #218
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.
Thanks for this! A few comments inline below.
For the tests, can you add one that explicitly does not have an alt attribute and assert that the parsed result is a string instead of the object?
Looking good, just a couple more tests to be updated to pass: |
@@ -536,6 +552,10 @@ public function language(DOMElement $el) | |||
|
|||
// TODO: figure out if this has problems with sms: and geo: URLs | |||
public function resolveUrl($url) { | |||
// If not a string then return. | |||
if (!is_string($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.
missing whitespace before {
@@ -634,7 +654,9 @@ public function parseP(\DOMElement $p) { | |||
public function parseU(\DOMElement $u) { | |||
if (($u->tagName == 'a' or $u->tagName == 'area' or $u->tagName == 'link') and $u->hasAttribute('href')) { | |||
$uValue = $u->getAttribute('href'); | |||
} elseif (in_array($u->tagName, array('img', 'audio', 'video', 'source')) and $u->hasAttribute('src')) { | |||
} elseif ( $u->tagName == 'img' and $u->hasAttribute('src') ) { |
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.
the whitespaces before/after the braces is WP coding style ;)
Pinging @gRegorLove as your previous comments re testing should all be addressed now :) |
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.
Looks good!
This is the first PR I've tried for this project. Having a little trouble with the testing.