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.

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

Replacing PSR-4 autoloading with a standard file map #721

Merged
merged 1 commit into from
Nov 5, 2014

Conversation

xmeltrut
Copy link
Contributor

@xmeltrut xmeltrut commented Nov 5, 2014

PSR-4 autoloading will not work because of the case difference (file is base but class is Base). This replaces it with a standard file map to include base.php with the Composer autoload.

bcosca added a commit that referenced this pull request Nov 5, 2014
Replacing PSR-4 autoloading with a standard file map
@bcosca bcosca merged commit f4e9eb0 into bcosca:master Nov 5, 2014
@francisdesjardins
Copy link

This seems cause a problem with Composer autoload and PHPUnit.

base.php is returning an instance of Base, which breaks the autoloader when used with PHPUnit.

PHPUnit test to reproduce the problem:

class FatFreeFrameworkTest extends PHPUnit_Framework_TestCase
{
    protected static $f3;

    public static function setUpBeforeClass()
    {
        self::$f3 = Base::instance();
    }

    public static function tearDownAfterClass()
    {
        self::$f3 = null;
    }

    public function testSomething() {
        $this->assertTrue(self::$f3 instanceof Base, 'Instance of Base');
    }
}

If you only have this test in the test suit you should see that there is no test to execute.

If you remove return Base::instance() from Base.php it works as intended

@ikkez
Copy link
Collaborator

ikkez commented Dec 9, 2014

i just saw another phpunit test on fat-free today. He doesn't seem to have that problem: https://github.com/creoLIFE/FatFree-Helpers/blob/master/test/HelpersTest.php
I'm not sure if the static call makes the difference or that he did not store the object in the test class.

@francisdesjardins
Copy link

It does not seem to use the autoload for F3

require_once( __DIR__ . '/../vendor/autoload.php');
require_once(__DIR__ . "/../vendor/bcosca/fatfree/lib/base.php");
require_once( __DIR__ . '/../FatFree/Helpers/Url.php');

There is an autoloader ... but F3 seems to be manually loaded ... You should be able to create a PHPUnit test using the config in the phpunit.xml without having to directly include files in the test case.

I think that @xmeltrut did the good thing, but for that to work with Composer autoload and PHPUnit, Base.php needs to get rid of the return Base::instance() at the end.

There will be a drawback if you are using something like $f3 = require('../lib/f3/base.php'); it would not work anymore since Base.php would no longer return an instance of Base.

@ikkez
Copy link
Collaborator

ikkez commented Dec 10, 2014

I doupt that the returning instance is to blame for that. Where does it cause a problem? Composer works with fat-free. So it rather looks like a bug in phpunit for me. Is phpunit overwriting the existing spl_autoloader? (i never tried it along with fatfree) Are you sure that the test works correct? Have you tried maybe this:

$this->assertTrue(\Base::instance() instanceof \Base, 'Instance of Base');

@francisdesjardins
Copy link

Test runs perfectly when you remove return Base::instance() from Base.php.

There is no logical explanation for a file containing a class definition to return something.

PHPUnit is using the Composer loader.

@bcosca
Copy link
Owner

bcosca commented Dec 10, 2014

This looks like a unit test gone wrong. Just because the class returns something doesn't mean it's illogical. It's just non-conformist. There is no rule book that says you can't or shouldn't do otherwise.

@francisdesjardins
Copy link

You can reproduce the problem without having any calls to F3 in an empty project ... just having it in Composer autoloader suffice to break PHPUnit tests.

phpunit.xml:

<?xml version="1.0" encoding="UTF-8"?>
<phpunit backupGlobals="false"
         backupStaticAttributes="false"
         bootstrap="vendor/autoload.php"
         colors="true"
         convertErrorsToExceptions="true"
         convertNoticesToExceptions="true"
         convertWarningsToExceptions="true"
         processIsolation="false"
         stopOnFailure="false"
         syntaxCheck="false">
    <testsuites>
        <testsuite name="Test Suite">
            <directory suffix=".php">./tests/</directory>
        </testsuite>
    </testsuites>
</phpunit>

PHPUnit test:

<?php
class FatFreeFrameworkTest extends PHPUnit_Framework_TestCase
{
    public function testSomething()
    {
        $this->assertTrue(true, 'That is true');
    }
}

Composer.json:

{
    "require": {
        "bcosca/fatfree": "dev-master"
    },
    "require-dev": {
        "bcosca/fatfree": "dev-master",
        "phpunit/phpunit": "4.4.*"
    }
}

You can try that in an empty project, so there is nothing more than F3 and PHPUnit in Composer autoloader and it will still crash the test suite.

@bcosca
Copy link
Owner

bcosca commented Dec 11, 2014

I suggest you take it up with the Composer team. It seems illogical that a third-party autoloader will have trouble with F3 considering spl_autoload_register is stackable, and any autoloader registered prior to F3 should have priority. F3 doesn't generate any exception or error in its autoload function either, so it conforms to the specs, if not a lot more flexible than the so-called standard (which forces you to follow certain namespace/file naming and invocation conventions). It should also be of no consequence whether the framework returns something or not when base.php is loaded. That is totally irrelevant to the job of the autoloader.

What your unit test demonstrates is a failure of the third-party autoloader (Composer's) to cope with a return value. Perhaps you should use F3's autoloader as the primary. It's a lot more forgiving than the standard.

@francisdesjardins
Copy link

Hi,

Been rechecking that issue and I have finally found the culprit, which could be either in F3 or PHPUnit.

So here is the code in F3

if (PHP_SAPI=='cli') {
    // Emulate HTTP request
    if (isset($_SERVER['argc']) && $_SERVER['argc']<2) {
        $_SERVER['argc']++;
        $_SERVER['argv'][1]='/';
    }
    $_SERVER['REQUEST_METHOD']='GET';
    $_SERVER['REQUEST_URI']=$_SERVER['argv'][1];
}

And the one in PHPUnit

public static function main($exit = true)
{
    $command = new static;

    return $command->run($_SERVER['argv'], $exit);
}

The use of $_SERVER['argv'][1] in both code causes the PHPUnit problem. When Base::instance() get called in base.php it initialize ``$_SERVER['argv'][1]value to/` and PHPUnit is using that to load the test suite.

Any suggestions ? I am not pointing F3 nor PHPUnit about that conflict.

@bcosca
Copy link
Owner

bcosca commented Jan 20, 2015

Since you're running the framework in CLI mode, there's really nothing we can do about it because the framework itself has a specific syntax and order of arguments when running from the command line. Altering this behavior will BREAK backward-compatibility, and that's something we don't intend to do.

@francisdesjardins
Copy link

That makes sense! Thank you for your time.

@bcosca
Copy link
Owner

bcosca commented Jan 20, 2015

That explains the misunderstanding. All the time I thought you were running it as a Web app, which doesn't connect the dots with PHPunit.

@xfra35 xfra35 mentioned this pull request Feb 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants