roave / backwardcompatibilitycheck Goto Github PK

class A { public function a() {} }
class A { public function a(int $a) {} }

class A { protected function a() {} }
class A { protected function a(int $a) {} }

class A { public function a() {} }
class A { public function a(int $a = 1) {} }

class A { protected function a() {} }
class A { protected function a(int $a = 1) {} }

final class A { public function a() {} }
final class A { public function a(int $a) {} }

final class A { protected function a() {} }
final class A { protected function a(int $a) {} }

but NOT:
final class A { public function a() {} }
final class A { public function a(int $a = 1) {} }

and NOT:
final class A { protected function a() {} }
final class A { protected function a(int $a = 1) {} }

class A { public function a() {} }
class A { protected function a() {} }

and

class A { protected function a() {} }
class A { private function a() {} }

and

class A { public function a() {} }
class A { private function a() {} }

PHP 7.2 only:
class A { public constant A = ''; }
class A { }

PHP 7.2 only:
class A { protected constant A = ''; }
class A { }

PHP 7.2 only, and NOT:
class A { private constant A = ''; }
class A { }

Any PHP version:
class A { constant A = ''; }
class A { }

We currently ship a massive single block of BC compliance rules hardcoded via dependency injection. It would be interesting to configure rules as following:

./bin/roave-backward-compatibility-check --rules=SomeInvokableClass

Where SomeInvokableClass implements BCRulesConfiguration or such.

Overall, I'd avoid weird .xml or such when not needed.

I installed this on a project that doesn't have a specified bin-dir

> vendor/bin/roave-backward-compatibility-check 
PHP Fatal error:  Uncaught RuntimeException: Could not find Composer autoload.php in /my/project/vendor/roave/backward-compatibility-check/bin/roave-backward-compatibility-check.php:292

Debugging the loop in roave-backward-compatibility-check.php it's searching the following locations:

/my/project/vendor/roave/backward-compatibility-check/bin/../vendor/autoload.php
/my/project/vendor/roave/backward-compatibility-check/bin/../autoload.php

Of course neither contain the project deps because they're all higher up in /my/project/vendor

We tackle the similar issue in phpspec by also looking at paths relative to getcwd() but it's not that great a solution.

At the least, you can document that bin-dir needs to exist and it needs to be one dir below the project root

We should return a non-zero exit code when BC breaks are found so CI builds will be failed.

from https://travis-ci.org/Roave/ApiCompare/jobs/372001598 :

$ bin/roave-backward-compatibility-check --from=master --to=HEAD
Comparing from 4b9138b628351653a8064b81bf2ee9ef062e0545 to 4b9138b628351653a8064b81bf2ee9ef062e0545...
In Process.php line 222:
                                                                               
  The command "'git' 'clone' '/home/travis/build/Roave/ApiCompare' '/tmp/api-  
  compare-4b9138b628351653a8064b81bf2ee9ef062e0545'" failed.                   
                                                                               
  Exit Code: 128(Invalid exit argument)                                        
                                                                               
  Working directory: /home/travis/build/Roave/ApiCompare                       
                                                                               
  Output:                                                                      
  ================                                                             
                                                                               
                                                                               
  Error Output:                                                                
  ================                                                             
  fatal: destination path '/tmp/api-compare-4b9138b628351653a8064b81bf2ee9ef0  
  62e0545' already exists and is not an empty directory.                       
                                                                               
roave-backwards-compatibility-check:assert-backwards-compatible [--from [FROM]] [--to TO] [--format [FORMAT]] [--] [<sources-path>]
The command "bin/roave-backward-compatibility-check --from=master --to=HEAD" exited with 1.

I'm assuming there's an automated way of generating a PHAR when we do a release, so if folks don't want to add to composer --dev they can just download from GitHub release.

Not that important for now, but maybe useful in future.

$ vendor/bin/roave-backward-compatibility-check roa:ass --to=HEAD --from=eb73ac50b890bf8 lib
Comparing from eb73ac50b890bf8c26115ff5975fc6adf093dd0b to 02538d3f95e88eb397a5f86274deb2c6175c2ab6...
Loading composer repositories with package information
Installing dependencies from lock file
Nothing to install or update
Generating optimized autoload files
Loading composer repositories with package information
Installing dependencies from lock file
Package operations: 2 installs, 0 updates, 0 removals
  - Installing doctrine/lexer (v1.0.1): Loading from cache
  - Installing doctrine/annotations (v1.6.0): Loading from cache
Generating optimized autoload files

In Fqsen.php line 48:

  "\Doctrine\Common\Reflection\boolean." is not a valid Fqsen.

Reproducible with the command above in doctrine/reflection repo (HEAD=02538d3f95e88eb397a5f86274deb2c6175c2ab6, but issue comes from --from commit).

$ git grep boolean
lib/Doctrine/Common/Reflection/StaticReflectionParser.php:     * @var boolean.
lib/Doctrine/Common/Reflection/StaticReflectionParser.php:     * @var boolean
lib/Doctrine/Common/Reflection/StaticReflectionParser.php:     * @param boolean              $classAnnotationOptimize Only retrieve the class docComment.

class A {}
final class A {}

Hyped of getting a cool changelog, I just tried this tool against doctrine/migrations 2.0.
Unfortunately the result is not really useful:
https://gist.github.com/Majkl578/2107492b128c35dff8d2007913d4cde8

Since the project namespace was renamed, almost everything is deemed to had been removed, few classes are detected as changed to interface. No method signature changes are reported at all.

It'd be cool to have some chance to provide hints to the tool to help detecting what was actually changed, since git isn't unfortunately useful in this case.

Current behavior

Everything is removed or changed to interface, no signature/type changes reported.

Wanted behavior

Providing hints file with some metadata:

• namespace was changed from X to Y
• class was renamed from X to Y
• method was renamed/moved from X to Y
• etc.

Note that these would still be (correctly) reported as BC breaks, but in a more useful way.

This is probably interesting for @Majkl578.

Given following `composer.json`
{
    "extra": {
        "branch-alias": {
            "develop": "3.x-dev",
            "master": "2.x-dev"
        }
    }
}
And a tag `2.2.0` exists
And `HEAD` is `master`
When we run `bin/roave-backward-compatibility-check`
Then the current version will be considered to be `2.99999.99999.99999.99999.99999`
And BC break analysis will be enforced

Given following `composer.json`
{
    "extra": {
        "branch-alias": {
            "develop": "3.x-dev",
            "master": "2.x-dev"
        }
    }
}
And a tag `2.2.0` exists
And `HEAD` is `develop`
When we run `bin/roave-backward-compatibility-check`
Then the current version will be considered to be `3.99999.99999.99999.99999.99999`
And BC break analysis will not be enforced

This kind of tweak will allow consumers of the package to just drop it into their .travis.yml, and it will encourage clean declaration of "branch-alias" for everyone to read and understand.

This should probably also be skippable with a flag, since we don't want the inferred version to be always detected, and sometimes we really always want a non-zero exit code in case of BC breaks, even if the branch alias declares a major version bump.

Also, this behavior should be skipped if --from is defined.

@nikolaposa this is also interesting for the version lib btw - unsure if the version lib can help with parsing branch aliases...

class A { public function a($a) {} }
class A { public function a(int $a) {} }

class A { public function a(int $a) {} }
class A { public function a(string $a) {} }

class A { protected function a($a) {} }
class A { protected function a(int $a) {} }

class A { protected function a(int $a) {} }
class A { protected function a(string $a) {} }

Would be nice to make Comparator yield errors like new ClassFinalized() or new ClassDeleted() instead of hardcoding strings + names.

On top of that, have a processor/formatter that can:
a) categorize the changes by namespace, type of change etc.,
b) write specific formats - pretty console, conservative xml, ugly yaml (yuck!) etc.

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?

As discussed with @asgrim in #77:

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?

This means that array is not a good solution for accumulating BC breaks, and instead yield and yield from must be used aggressively to iterate over the results as they get produced, and inform the user about each step in the process by continuing to produce output.

Currently signature comparisons are hardcoded here.
Would be nice to make them reusable so maybe tools like PHPStan can reuse it later. Just an abstract API like compareMethods($a, $b) etc.

As per #38 (comment), we need to also work on trait. This is low priority for now, but it will eventually come up in the next major (after 1.0.0)

Currently in #15 we do no rev-parsing at all so you have to provide full commit hash. At same time, make --to optional with default value of HEAD

Steps to reproduce:

1. check out phpspec/phpspec
2. add this tool to composer requires and update
3. run vendor/bin/roave-backward-compatibility-check

Error:

[Symfony\Component\Process\Exception\ProcessFailedException]                     
  The command "'git' 'rev-parse' ''" failed.                                       
                                                                                   
  Exit Code: 128(Invalid exit argument)

The input parameter --from is optional so its default value is NULL. this is explicitly cast to a string '' in AssertBackwardsCompatible::parseRevisionFromInput, but the implementation of GitParseRevision then builds a the CLI with an empty string, which breaks as above

As discussed in person with @asgrim and @akrabat, the following change is not a BC break, because final already makes it impossible to rely on protected properties:

final class Foo
{
    protected $bar = 'baz';
}

changed into:

final class Foo
{
    private $bar = 'baz';
}

This kind of scenario is tricky to represent correctly with the current design of the Comparator

As per #13 mwop mentioned markdown output would be useful for adding to tag messages/changlogs etc.

class A { public function a() {} }
class A { public function a(): int {} }

class A { protected function a() {} }
class A { protected function a(): int {} }

but NOT (possibly?)

final class A { function a() {} }
final class A { function a(): int {} }

Add stuff for the CS checks

This package can both be used as an application and as a dependency, so adding roave/security-advisories to require is quite a heavy transitive dependency.

Should be moved to require-dev so that it is only active when the package is used as an application.

PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 9437184 bytes) in vendor/composer/composer/src/Composer/Repository/ComposerRepository.php on line 565

Default configuration for php cli runtime is 128M.

For larger code bases it's very likely to hit this limit easily. Fixed the issue locally by disabling the memory limit in the bin/roave-backward-compatibility-check.php with ini_set('memory_limit', '-1');

PHP 7.2 only:
class A { public constant A = ''; }
class A { protected constant A = ''; }

PHP 7.2 only:
class A { protected constant A = ''; }
class A { private constant A = ''; }

PHP 7.2 only, and NOT:
class A { public constant A = ''; }
class A { private constant A = ''; }

#15 (comment)

In #15 the /src/ path is hard-coded, which is a bad assumption.

The easiest from our perspective is to provide a param like --src-path=src or something, although @macnibblet suggested examining composer.json to look for the autoload paths which could be interesting...

Hey there,

Great tool! Excited to use this!
I tried running this on https://github.com/api-platform/core on the 2.2 branch:
bin/roave-backward-compatibility-check --from=v2.2.6 --to=HEAD
but receive:

In IdentifierNotFound.php line 30:

  Roave\BetterReflection\Reflection\ReflectionClass "Doctrine\DBAL\Types\Type" could not be found in the located source

Couldn't immediately work out the problem to fix myself, so tracking an issue.

Although PR #60 works, @Ocramius mentioned that all the anon classes are detected as "removed classes" and that the anon classes should be removed from the "defined API" up front.

I've noticed when explaining what the tool does, folks jump to conclusion that this is used for comparing HTTP/REST API versions. I wonder if renaming the package could alleviate this problem. Would be better to do it before we tag anything as naturally it's (ironically) a big BC break ;)

Not sure WHAT to suggest though for naming, feedback welcome on this.

Currently the path to the repo is hardcoded to getcwd(). Fix this proper-like (default to getcwd() but allow specifying --repository or something

At the moment no colours, @macnibblet suggested adding them in #15 (comment)

yes, I really said that ◉_◉

The idea is that a BC break should be detected (following the mantra "when in doubt: something is wrong"), and the exception message collected and shown in the report, but without crashing further execution.

#38 (comment)

class A { public function a() {} }
class A { public static function a() {} }

class A { public static function a() {} }
class A { public function a() {} }

Once #15 is merged, write some code that detects the last minor version automatically from git porcelain and make the --from param.

Yeah.

class A { public $a; }
class A { protected $a; }

and

class A { protected $a; }
class A { private $a; }

and

class A { public $a; }
class A { private $a; }

Ref: #35 (comment)

This isn't actually executed in the CI build, and it should be added.

So far we have not defined or even discussed what kind of usage this tool has. Creating this issue to discuss and formalise usage, before 1.0.0.

As per findings in #76, the STDERR and STDOUT get mixed with each other regardless what we do.

This is likely an effect of combining the composer/composer CLI output with the symfony/console output, which is probably not a scenario that symfony/console was designed for.

Ref: #76 (comment)

As per recent finding, the comparisons are not currently limited to src/ (or whatever source path is given), but all classes are being checked.

This means that diffing a codebase will likely cause diffing of all detected classes anywhere if we ever include autoloading of other symbols.

A simple applicable restriction is to only consider classes that have a file path in the defined source directory.

class A { public $a; }
class A { }