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.

Sign up for GitHub

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

Jump to bottom

Improve export of closed resources #36

Closed
jrfnl opened this issue Nov 11, 2021 · 2 comments · Fixed by #37
Closed

Improve export of closed resources #36

jrfnl opened this issue Nov 11, 2021 · 2 comments · Fixed by #37

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Nov 11, 2021

Given the following code:

use SebastianBergmann\Exporter\Exporter;

include __DIR__ . '/vendor/autoload.php';

$exporter = new Exporter;

$dir = opendir( __DIR__ );
echo $exporter->export($dir), PHP_EOL;

closedir($dir);
echo $exporter->export($dir), PHP_EOL;

... the output would be along the lines of:

resource(11) of type (stream)
NULL

... which when used in a PHPUnit error message results is something along the lines of:

Failed asserting that NULL is not of type "boolean"

I'd like to suggest reporting closed resources as resource (closed) instead of NULL.
@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 11, 2021

(PR upcoming)

@jrfnl jrfnl mentioned this issue Nov 11, 2021
Merged
jrfnl added a commit to Yoast/PHPUnit-Polyfills that referenced this issue Nov 11, 2021
@jrfnl
AssertClosedResource: improve error messages
a1f42fd 
The `sebastian/exporter` package has been a dependency of PHPUnit from before PHPUnit 4.8.x (minimum supported version by this library), so it is safe to use the same variable export methodology as PHPUnit itself uses.

The class is prefixed via PHP_Scoper when it is includes as part of a PHPUnit Phar file and the toggle added will make sure the class can be loaded both when run via a Composer install, as well as when run via a Phar file.

Also note that the Exporter reports closed resources as `NULL`. This has been reported upstream via sebastianbergmann/exporter#36 and for the polyfill a fix has been put in place to work around this.
This was referenced Nov 11, 2021
Merged
Merged
@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 11, 2021

As both PRs have now been merged and the 4.x branch merged to master, I think this issue can be closed ?

jrfnl added a commit to Yoast/PHPUnit-Polyfills that referenced this issue Nov 11, 2021
@jrfnl
AssertClosedResource: improve error messages
d7c0982 
The `sebastian/exporter` package has been a dependency of PHPUnit from before PHPUnit 4.8.x (minimum supported version by this library), so it is safe to use the same variable export methodology as PHPUnit itself uses.

The class is prefixed via PHP_Scoper when it is includes as part of a PHPUnit Phar file and the toggle added will make sure the class can be loaded both when run via a Composer install, as well as when run via a Phar file.

Also note that the Exporter reports closed resources as `NULL`. This has been reported upstream via sebastianbergmann/exporter#36 and for the polyfill a fix has been put in place to work around this.

**Important**:
While the `sebastian/exporter` package has , in effect, now become a direct dependency of the Polyfills, I'm not going to declare it as such in the `composer.json` file as for all supported PHPUnit versions, the package will already be available via PHPUnit itself, whether installed via Composer or as a PHAR.
This prevent potential version conflicts when tests are run via the PHAR, while the Polyfills have been installed via Composer and saves hassle of having to take care of autoloading the Exporter file, while it will be loaded for PHPUnit itself anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve export of closed resources jrfnl/exporter
2 participants
@sebastianbergmann @jrfnl