Recommend running `maglnet/composer-require-checker` before using this tool

[Ocramius]

Scenario

namespace MyLib;

use External\Foo;
use External\Bar;

class MyClass extends Foo, implements Bar {}

Assuming MyClass is part of our package.
Assuming External\Foo and External\Bar are from require-dev or completely missing.

roave/backward-compatibility-check will not be able to compare two versions of this class, as it will generate two empty stubs:

• namespace External {interface Foo{}}
• namespace External {interface Bar{}}

These are invalid, since the extends statement does not allow interfaces.

The stubbing mechanism also has no way to determine if Foo and Bar are to be classes or interfaces, since no requirement is passed to it (and it is not possible to do that anyway).

The problem

The root problem is not the stubbing, which is really just a mechanism to prevent basic crashes for rare optional dependency scenarios. The issue lies with how we treat dependencies in first place. If something is in our sources and we rely on external classes, said external classes must be part of the "require" section of composer.json before proceeding.

Further issues with missing "require"

Consider following example:

class ComparedClass
{
    public function changedMethod(A $a) : B;
}

If A and B are unknown to the system, a change in a dependency can lead to downstream BC breaks due to BC breaks in A and B themselves, specifically in variance and contravariance. Stubbing out these two symbols can indeed simplify operations at first, but can also shadow issues when the libraries where A and B come from change.

Related: #74

Proposed solution

In order to raise awareness of the problem and improve the overall ecosystem, I endorse that downstream consumers of this library also run maglnet/composer-require-checker before running this tool, or if this tool crashes on undefined symbols. This is a documentation issue (and this ticket is to be transformed into a documentation page).

/cc @maglnet maybe you can suggest how to make this user-friendly?

[asgrim]

Is it really in the scope of BCC to make sure all the deps are there? I'm leaning to "no", and I'm not a fan of the stubbing idea either. We can't predict all the requirements of every project, it should be up to the project developer to make sure all deps exist when we are run. If maglnet/composer-require-checker helps, then sure make a recommendation it is run first - but having all deps available should be a documented prerequisite, we shouldn't try and fix the world's dependency issues...

[Ocramius]

@asgrim agree: what I'm thinking of for the immediate future is to make fail-safe checkers that try{}catch(\Throwable $e) {} and record them into a Change instance that then displays an error.

Not the best thing in the world, but it would allow to at least run through the entire analysis without a hard crash...

[asgrim]

That's acceptable I think... Or maybe simply collect errors and display at the end? Thinking a summary output like phpunit/phpcs like:

.......B......E......
.....B............EEE

Where B is a break and E is an error.. then display them at the end?

[asgrim]

(not necessarily add the output, was just an example btw)

[Ocramius]

Could also work, would need to work with iterators only though.
That is probably the correct solution indeed, as otherwise the tool also simply hangs for ages before producing any output :-)

[maglnet]

Is it a typical situation that there are empty stubs?
If not, one could raise a warning like There are empty stubs, did you check your dependencies? and give a hint to maglnet/composer-require-checker.

[asgrim]

to be clear: I think the stubbing should be removed; it should be a hard fail like @maglnet said:

did you check your dependencies? and give a hint to maglnet/composer-require-checker

Another proposal: hard-require maglnet/composer-require-checker and run it beforehand? (yes, ties up and forces an extra requirement, but it may help resolve this?)

[bendavies]

just to repeat the usual usecase for this type or thing, so the discussion is not lost:
often libraries will have optional "suggested" dependencies, which, if installed by the consumer, will enable extra features in the library (via class exists checks).
The library itself will install the suggestion in require-dev for testing purposes.

[Ocramius]

which, if installed by the consumer, will enable extra features in the library (via class exists checks).

Yeah, but those should really be segregated into optional installable packages that requires both dependencies. Yes, that will lead to an explosion of packages, but in ZendFramework specifically we constantly get issues reported due to optional dependencies.