Skip to content

Conversation

@remicollet
Copy link
Collaborator

@remicollet remicollet commented Oct 22, 2016

For discussion.

  • Using exception seems a good idea.

At least it allow better test suite.
With this change, coverage is 100% for Dependency.php (~90% for Autoload.php)

  • drop needed to ensure class if not already loaded in each autoloader (move it to main one)

@remicollet remicollet changed the title throw exception instead of fatal error Various fixed and small improvements Oct 25, 2016
@remicollet remicollet changed the title Various fixed and small improvements Various fix and small improvements Oct 25, 2016
@remicollet
Copy link
Collaborator Author

Fedora package (in rawhide) already have c781f66 and 9e2b39c

  • composer and its dependencies use it
  • PHPUnit use it
  • php-fedora-autoloader now build correctly with PHPUnit
  • php-symfony FTBFS is fixed

@siwinski have you time to review all this set of changes ? (perhaps time for a 0.2.0)

@siwinski
Copy link
Member

@remi -- The biggest question I have regarding this is if we consider the following OK?:

try {
    \Fedora\Autoloader\Dependencies::required(array(
        '/a/required/dependency/that/does/not/exist/and/is/ignored.php',
    ));
} catch (Exception $e) {
}

@remicollet
Copy link
Collaborator Author

remicollet commented Oct 25, 2016

Indeed, using exception gives this feature (catching it)
I think this is OK, and a good improvement (best practices)

@siwinski siwinski merged commit 21f1322 into master Oct 25, 2016
@siwinski siwinski deleted the excep branch October 27, 2016 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants