Rewrite addMedia to treat remote URLs correctly, use fewer regexes #96
Conversation
…ixes jeroendesloovere#87, jeroendesloovere#94 Also fix outstanding phpcs phpdoc complaints
src/VCard.php
Outdated
| if (filter_var( | ||
| $url, | ||
| FILTER_VALIDATE_URL, | ||
| FILTER_FLAG_SCHEME_REQUIRED | FILTER_FLAG_HOST_REQUIRED |
There was a problem hiding this comment.
FILTER_FLAG_SCHEME_REQUIRED | FILTER_FLAG_HOST_REQUIRED are unnecessary because this project requires php version 5.3.3 or later and since version 5.2.1, FILTER_FLAG_SCHEME_REQUIRED and FILTER_FLAG_HOST_REQUIRED are defaults.
http://php.net/manual/en/filter.filters.validate.php: 5.2.1 FILTER_VALIDATE_URL now defaults to FILTER_FLAG_SCHEME_REQUIRED and FILTER_FLAG_HOST_REQUIRED.
There was a problem hiding this comment.
Good catch, thanks.
src/VCard.php
Outdated
| if (strpos($mimeType, ';') !== false) { | ||
| $mimeType = strstr($mimeType, ';', true); | ||
| } | ||
| if (!is_string($mimeType) or substr($mimeType, 0, 6) != 'image/') { |
There was a problem hiding this comment.
Use || instead of or.
You can safely use !== instead of !=
There was a problem hiding this comment.
I really don't like using ||/&& instead or or/and. It's so much less readable, and the only time the operator precedence difference has a meaningful effect is during assignments - where the far more common mistyping/misreading of | or & produces likely worse outcomes, much like = vs ==. It's also against most frameworks' coding standards (that express a preference e.g. CI) for the same reasons.
!== is 🤷♂️ in this case.
There was a problem hiding this comment.
As described in https://github.com/jeroendesloovere/vcard/blob/master/README.md#pull-requests: "Coding Syntax - Please keep the code syntax consistent with the rest of the package.", you must have the code syntax the same consistent with the rest of the package.
If you search in the code you will never find 'or' or 'and'. Only || and && are used.
Personally I find || and && just good practice and easier to use. Also, if you use a good idea like PhpStorm, you will see errors instantly.
!== is also a case of good practice. If you can use strict, use it.
There was a problem hiding this comment.
That's fair. I do use PHPStorm, and it flags && as a standards contravention in my current config. There's a difference between good and common.
tests/VCardTest.php
Outdated
| @@ -163,7 +173,7 @@ public function testAddUrl() | |||
| * Test adding photo with no value | |||
| * | |||
| * @expectedException JeroenDesloovere\VCard\VCardMediaException | |||
| * @expectedExceptionMessage Nothing returned from URL. | |||
| * @expectedExceptionMessage Returned data is not an image. | |||
| */ | |||
| public function testAddPhotoWithNoValue() | |||
| { | |||
There was a problem hiding this comment.
Perhaps you might be able to fix a test related to the addMedia function at line 187 of https://github.com/jeroendesloovere/vcard/pull/96/files#diff-fc15453e7e481d3e92f86a7d1d2e7a99R187. "@t@github.com:jeroendesloovere/vcard.gitexpectedExceptionMessage Nothing returned from URL." has to be replace with "@expectedExceptionMessage Returned data is not an image." so the test is correct. If it is not possible then I will make a ticket of it.
There was a problem hiding this comment.
I did have a think about that. The problem is that we know that it's not an image before we attempt to fetch its content (and thus before we know that nothing is returned), so the original test doesn't make sense any more.
There was a problem hiding this comment.
In that case I would say take the test away.
There was a problem hiding this comment.
It can fail two different ways: we could have something that says it's not an image at all and fails early (why the test needed changing), or we could have something that says is it an image, but proves to be empty when fetched. To test that we'd need to have an image file that returns a correct image MIME type, but with zero-length content. I've added a file and a test for that; the URL for the file will need changing after merge to point at the master repo rather than mine.
Add test for empty image file
|
Hi guys, is this PR ready to be merged, or are there things that need to be fixed: Note to self:
|
|
It's ok as far as I'm concerned. I think there might be a few happy people if you merged it :) |
|
Thanks @Synchro for the PR and @to-be-the-best for your review. |
Fixes #87 and #94
Includes accompanying tests
Also fixes outstanding phpcs phpdoc complaints