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

Allow custom poster images #103

Merged
merged 17 commits into from Nov 12, 2013
Merged

Allow custom poster images #103

merged 17 commits into from Nov 12, 2013

Conversation

mfairchild365
Copy link
Contributor

This adds a poster field to the media table that will override the default iTunesU poster image.

@ghost ghost assigned saltybeagle and kabel Nov 8, 2013

$mediahub = new UNL_MediaHub($dsn);

$list = new UNL_MediaHub_MediaList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of iterating over the entire table of media, it would be better to have a filtered list of only media with a poster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfairchild365
Copy link
Contributor Author

@kabel ready for another review

}

foreach ($http_response_header as $header) {
if (strpos($header, 'HTTP/1.1 404 Not Found') !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The status header will always be the first in the array (no need to check the others). Also, not all servers will respond with HTTP/1.1. Also, servers will not always use the string Not Found. Only the response code is guaranteed by the protocol.

404 is also not the only problematic response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very true. I was just copy/pasting the find404Media script. Both could use an update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be better to look for 200 instead of HTTP/1.1 404 Not Found. If its not 200 remove it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That would be better and can be easily parse by exploding the first header on whitespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The status header will always be the first in the array (no need to check the others). Also, not all servers will respond with HTTP/1.1. Also, servers will not always use the string Not Found. Only the response code is guaranteed by the protocol.

404 is also not the only problematic response.
@mfairchild365
Copy link
Contributor Author

@kabel ready for another review

kabel added a commit that referenced this pull request Nov 12, 2013
@kabel kabel merged commit 4a3e455 into unl:master Nov 12, 2013
mfairchild365 pushed a commit to mfairchild365/UNL_MediaHub that referenced this pull request Apr 6, 2016
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.

None yet

3 participants