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

FilteredIterator: fix PHP 5.2/5.3 compatibility and actually run the tests #457

Merged
merged 3 commits into from
Apr 16, 2021

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Mar 20, 2021

FilteredIterator: fix PHP 5.2 compatibility and stabilize the code

The $callback passed to the constructor of the FilteredIterator class may not be callable and if so, would cause a fatal error.

This puts some defensive coding in place to guard against that and adds a unit test for it as well.

This also fixes the tests failing on PHP 5.2, as when the object is unserialized on PHP 5.2, the $callback property will no longer be set. This looks to have been a bug in PHP 5.2 which was fixed in PHP 5.3.

Tests/FilteredIterator: fix a parse error on PHP 5.3

Even though the code is wrapped in a version check for running, the code still needs to parse correctly on low PHP versions.

This was not the case now, which could break the running of the tests of PHP 5.3.

Fixed now.

Tests: actually run the test for the FilteredIterator

As all tests are explicitly named in the test configuration, the test for the FilteredIterator as added in #422 were not being run as they hadn't been added to the config.

This fixes that.

@jrfnl jrfnl added this to the 1.8.0 milestone Mar 20, 2021
@jrfnl jrfnl requested a review from schlessera March 20, 2021 17:30
@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #457 (3621755) into master (18cc33e) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #457      +/-   ##
============================================
- Coverage     93.06%   93.01%   -0.06%     
- Complexity      764      765       +1     
============================================
  Files            21       21              
  Lines          1788     1789       +1     
============================================
  Hits           1664     1664              
- Misses          124      125       +1     
Impacted Files Coverage Δ Complexity Δ
library/Requests/Utility/FilteredIterator.php 80.00% <100.00%> (+2.22%) 4.00 <0.00> (+1.00)
library/Requests/Transport/cURL.php 94.23% <0.00%> (-0.49%) 67.00% <0.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 18cc33e...3621755. Read the comment docs.

@jrfnl
Copy link
Member Author

jrfnl commented Mar 20, 2021

I've added a second commit to enable actually running the test....

@jrfnl
Copy link
Member Author

jrfnl commented Mar 20, 2021

The build against PHP 5.2 is failing for this PR and I'm wondering whether to spend time on fixing the test for this or to just skip this particular test on PHP 5.2.

@schlessera Opinion ?

@jrfnl jrfnl force-pushed the feature/fix-php-5.3-compatibility branch from 00a63bf to de177d0 Compare March 20, 2021 22:13
@jrfnl jrfnl changed the title Tests/FilteredIterator: fix a parse error on PHP 5.3 FilteredIterator: fix PHP 5.2/5.3 compatibility and actually run the tests Mar 20, 2021
The `$callback` passed to the constructor of the `FilteredIterator` class may not be callable and if so, would cause a fatal error.

This puts some defensive coding in place to guard against that and adds a unit test for it as well.

This also fixes the tests failing on PHP 5.2, as when the object is unserialized on PHP 5.2, the `$callback` property will no longer be set. This looks to have been a bug in PHP 5.2 which was fixed in PHP 5.3.
Even though the code is wrapped in a version check for running, the code still needs to parse correctly on low PHP versions.

This was not the case now, which would break the running of the tests of PHP 5.3.

Fixed now.
As all tests are explicitly named in the test configuration, the  test for the `FilteredIterator` as added in 422 were not being run as they hadn't been added to the config.

This fixes that.
@jrfnl
Copy link
Member Author

jrfnl commented Mar 20, 2021

Okay, fixed it as the fix which is needed for 5.2, actually makes sense for all PHP versions anyhow.

So I've added (yet another) commit and updated the PR description to cover all three changes.

@jrfnl jrfnl force-pushed the feature/fix-php-5.3-compatibility branch from de177d0 to 3621755 Compare March 20, 2021 22:16
@jrfnl
Copy link
Member Author

jrfnl commented Mar 20, 2021

This is now ready for merge - passing build: https://travis-ci.org/github/WordPress/Requests/builds/763756355

PHP 8 is failing as discussed in #439. Doesn't really matter as nightly is currently 8.0.3-dev and passing.

@schlessera schlessera merged commit 30c5d05 into master Apr 16, 2021
@schlessera schlessera deleted the feature/fix-php-5.3-compatibility branch April 16, 2021 10:17
@jrfnl jrfnl mentioned this pull request Apr 18, 2021
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.

2 participants