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

Throw exception if invalid filename is provided #379

Merged
merged 5 commits into from
Aug 20, 2021

Conversation

datagutten
Copy link
Contributor

Throw an exception if an invalid path is provided in the filename option.
Before this change, the script will continue and try to write and close a file pointer which does not exist.

@codecov-io
Copy link

Codecov Report

Merging #379 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #379      +/-   ##
============================================
+ Coverage     92.11%   92.14%   +0.03%     
- Complexity      760      763       +3     
============================================
  Files            21       21              
  Lines          1762     1770       +8     
============================================
+ Hits           1623     1631       +8     
  Misses          139      139              
Impacted Files Coverage Δ Complexity Δ
library/Requests/Transport/cURL.php 93.77% <100.00%> (+0.09%) 67.00 <0.00> (+1.00)
library/Requests/Transport/fsockopen.php 94.28% <100.00%> (+0.09%) 69.00 <0.00> (+1.00)
library/Requests/Cookie.php 99.35% <0.00%> (+<0.01%) 63.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a009398...c1eb6dd. Read the comment docs.

@jrfnl
Copy link
Member

jrfnl commented Aug 13, 2021

@datagutten Hiya! Sorry for the long wait, but we're finally trying to get through the list of open PRs.

We're currently planning two triage sessions for Requests 2.0 and would like to invite you to join us in one or both of these sessions to talk us through the PR and discuss it.

The triage sessions are currently planned for:

  • Friday August 20, 07:00 - 11:00 UTC
  • Friday September 3, 07:00 - 11:00 UTC

Would you be available to join us during those times on one of those days ?

library/Requests/Transport/cURL.php Outdated Show resolved Hide resolved
library/Requests/Transport/fsockopen.php Outdated Show resolved Hide resolved
@jrfnl
Copy link
Member

jrfnl commented Aug 20, 2021

  • Rebased the PR to make it mergable.
  • Added an extra commit with minor tweaks after review.

@jrfnl jrfnl added this to the 2.0.0 milestone Aug 20, 2021
* Move silence operator to the function call it applies to and ignore the CS error.
* Spacing around strict equals comparison operators and other minor CS fixes.
* Use the PHPUnit `expectException...()` methods.
* Update the expected exception message to be in-line with what was in the docblock as the messages from cURL and fsockopen differ.
* The `testStreamToNonWritableFile()` already tested another aspect of trying to write a data stream to an unwritable file, so had to be adjusted to account for the new exception.
* Minor tweak to the `testStreamToInvalidFile()` expected exception message as this can vary slightly across PHP versions.

Includes adding a docblock to both tests to clarify the difference between the tests.
@jrfnl
Copy link
Member

jrfnl commented Aug 20, 2021

Added a second commit with minor tweaks to the test and adjustment to an existing test to allow for the exception. This should now be good to go.

@jrfnl jrfnl requested a review from schlessera August 20, 2021 15:51
@schlessera schlessera merged commit 746338d into WordPress:develop Aug 20, 2021
@datagutten datagutten deleted the fopen-exception branch October 30, 2021 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants