[9.0.x] Error is unintuitive when there are no files to lint

[hemberger]

I was looking for the equivalent of the previous option --no-files-exit-code, but it looks like it was removed in v9. While testing that it still properly errors when there are no files, I found a less intuitive error message than I was expecting in my programmatic usage:

LogicException: You must call one of in() or append() methods before iterating over a Finder.

vendor/symfony/finder/Finder.php:659
vendor/symfony/finder/Finder.php:736
vendor/overtrue/phplint/src/Linter.php:86

Would it be possible to provide a clearer error message in this case? Please let me know if I can provide any further details. Thanks!

[llaville]

Yes. What do you really expected ? Message / behavior ?

[hemberger]

Sorry for being unclear. I thought it would be helpful to have an error message like "Could not find any files to lint". Thanks again.

[llaville]

Could you produced your API script, and specify which PHPLint version that raise such condition please ?

I did not be able to reproduce it locally, either with console command or program script like into the examples folder !

[llaville]

With console condition , when path did not contains any PHP files :

[hemberger]

Thanks for looking into it. While trying to reproduce the error, I realize that I made a mistake in reporting this issue. Sorry about that! Let me outline the two cases I've now verified:

1. If the specified path argument does not exist, then you'll get the error as reported above ("You must call one of in() or append() methods before iterating over a Finder."). Could we instead throw an exception with a message such as "The input path <path> does not exist"? This would make it clear to the user what the exact nature of the problem is.
2. If the specified path does exist, but no files are found, then it exits without any errors. In this case, an error such as "Could not find any files to lint" would be very helpful, since it is highly suggestive of a mistake in configuration, and can be easily overlooked if the process reports success.

I'm using version 9.0.3 programmatically within a PHPUnit test case, but here's a script that can reproduce the behavior:

<?php declare(strict_types=1);

use Overtrue\PHPLint\Command\LintCommand;
use Overtrue\PHPLint\Configuration\ConsoleOptionsResolver;
use Overtrue\PHPLint\Event\EventDispatcher;
use Overtrue\PHPLint\Finder;
use Overtrue\PHPLint\Linter;
use Symfony\Component\Console\Input\ArrayInput;

require_once('vendor/autoload.php');

$dispatcher = new EventDispatcher([]);

$arguments = [ 
    'path' => [__DIR__ . '/empty_dir'], // or '/missing_dir'
    '--no-configuration' => true,
];  
$command = new LintCommand($dispatcher);
$input = new ArrayInput($arguments, $command->getDefinition());
$configResolver = new ConsoleOptionsResolver($input);

$finder = new Finder($configResolver);
$linter = new Linter($configResolver, $dispatcher);
$files = $finder->getFiles();
$results = $linter->lintFiles($files);

[llaville]

First, thanks for reporting with such details (I appreciate a lot) !

Second, concerning this point :

If the specified path argument does not exist, then you'll get the error as reported above ("You must call one of in() or append() methods before iterating over a Finder."). Could we instead throw an exception with a message such as "The input path does not exist"? This would make it clear to the user what the exact nature of the problem is.

👍 We should be aware by an Exception that the path analyzed does not exists !

Third, concerning this point :

If the specified path does exist, but no files are found, then it exits without any errors. In this case, an error such as "Could not find any files to lint" would be very helpful, since it is highly suggestive of a mistake in configuration, and can be easily overlooked if the process reports success.

👎 Console command already display it (see my previous screenshot), and programmatically, you must check it yourself, because everyone is free to handle this case as they want.

$results = $linter->lintFiles($files);

if (count($results->getMisses()) === 0 && count($results->getHits()) === 0) {
    throw new Exception("Could not find any files to lint");
}

[llaville]

Strategy changed : No more Exception ! As Console Command already display a warning when there are no files to lint (whatever path is empty or does not exists) since version 9.0.0

Results will be :

For console (warning message changed a little)

Programmatically, developer is in charge to check what is wrong (in his implementation logic)

[hemberger]

I am very unhappy with this resolution. I can't imagine any scenario where you'd want to return successfully when the lint directory is missing or no files are found. What would even be the point of using phplint in that case?

I also don't understand why you wouldn't want parity with the command-line usage. If the default is to validate meaningful input paths, wouldn't you want users of the programmatic interface to get the same behavior? The current behavior makes it easy to use incorrectly, which is dangerous.

What if you specify two paths, but only one of them is missing (perhaps due to a typo)? Is there a value to allowing the caller to make this mistake, and perhaps have files go unlinted?

[llaville]

I am very unhappy with this resolution. I can't imagine any scenario where you'd want to return successfully when the lint directory is missing or no files are found. What would even be the point of using phplint in that case?

You're probably the first one to be unhappy with such situation.
Let me explains with a bit of project history when I was not yet contributor.
Have you tried oldest versions ? For example pick-up the 5.3.0 version and try it.

Both console command (with tests/fixtures/missing_dir) and programmatically (with app) gave the same results.
Both directories does not exists.

<?php
require_once __DIR__ . '/vendor/autoload.php';

use Overtrue\PHPLint\Linter;

$path = __DIR__ .'/app';
$exclude = ['vendor'];
$extensions = ['php'];
$warnings = true;

$linter = new Linter($path, $exclude, $extensions, $warnings);

// get errors
$errors = $linter->lint();

var_dump($errors);
/*

array(0) {
}

 */

I also don't understand why you wouldn't want parity with the command-line usage.

I'm sorry, but it's the same behavior between command line and PHP script !

Older versions like 5.3.0 and new one 9.0.x did not raise any exception to notify user of invalid input paths.

[hemberger]

I'm sorry, but it's the same behavior between command line and PHP script !

That doesn't appear to be the case to me, though. With command-line usage, you get a warning that there are no files to lint; but with programmatic usage, the caller is responsible for performing that check. Am I misunderstanding?

My opinion is that testing tools should be as safe as possible by default, and provide options to override guard rails, if necessary. Requiring that users implement their own guard rails is asking for trouble.

Hopefully that argument makes some sense, but of course you should do what you think is best, regardless of my opinions. :) Thanks for taking the time to listen.