Skip to content

changed file_exists to file to allow urls#56

Merged
char0n merged 5 commits into
char0n:masterfrom
ToshY:bug/access-urls-exception
Dec 20, 2021
Merged

changed file_exists to file to allow urls#56
char0n merged 5 commits into
char0n:masterfrom
ToshY:bug/access-urls-exception

Conversation

@ToshY

@ToshY ToshY commented Dec 19, 2021

Copy link
Copy Markdown
Contributor

Fixes #55

@char0n char0n left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for contributing! Please have a look at my code review comments. Also if we're introducing this feature, it would be good to add tests for it. Please use this testing file for testing: https://github.com/char0n/ffmpeg-php/blob/master/tests/data/test.mp4?raw=true. I can imagine following tests:

  • testing if FFMpegProvider can handle URL
  • testing if FFProbeProvider can handle URL
  • testing if Movie can handle URL
  • testing what happens when provided URL cannot be fetched

Comment thread src/OutputProviders/FFMpegProvider.php Outdated
Comment thread src/OutputProviders/FFProbeProvider.php Outdated
@ToshY

ToshY commented Dec 19, 2021

Copy link
Copy Markdown
Contributor Author

Well the CI seems to be fine with it, but I honestly don't know if it's okay like this, as I normally don't write tests. So I just tried to mimic the other tests and ended up with this.
Let me know if there's something that needs to be changed.

@char0n char0n left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The tests satisfy the requirements. Nicely done. I'll just do some minor cleanup in them before the release.

@char0n
char0n merged commit 022e95a into char0n:master Dec 20, 2021
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.

Publicly accessible URLs throw exception

2 participants