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

Update AppKernel.php #114

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update AppKernel.php #114

wants to merge 4 commits into from

Conversation

Aliance
Copy link

@Aliance Aliance commented Apr 21, 2017

Without namespace IDE throws an error of duplicated class:
2017-04-21 12 15 21

@stof
Copy link

stof commented Apr 21, 2017

you need to update the testsuite to make it use the namespaced kernel though

*/
protected static function getKernelClass()
{
require_once __DIR__.'/../app/AppKernel.php';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even better: configure the autoload_dev in composer.json to be able to autoload this class (I think it is even already able to do it), making this requiring useless

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is even already able to do it

Seems to be true.

@Aliance
Copy link
Author

Aliance commented Apr 21, 2017

My local tests passed:

~/www/Aliance/ExcelBundle $ ./bin/phpunit 
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

.........

Time: 237 ms, Memory: 17.75MB

OK (9 tests, 19 assertions)

Test fails at travis in PHP 5.3 🤔 , seems to be the same behavior in master?

@Aliance
Copy link
Author

Aliance commented Apr 21, 2017

@liuggio review please

@liuggio
Copy link
Owner

liuggio commented Apr 21, 2017

@Aliance thanks a lot!

the problem is on “[“ on <5.4 PHP version :(
I'll look into that ASAP...

@Aliance
Copy link
Author

Aliance commented Apr 29, 2017

@liuggio did you have enough time to watch out the problem?

@Aliance
Copy link
Author

Aliance commented May 8, 2017

@liuggio hello. Sorry for being annoying but may be merge this pr?

@liuggio
Copy link
Owner

liuggio commented May 8, 2017

Sorry for the delay but this needs a major version, and this is not a killer feature :(

@Aliance
Copy link
Author

Aliance commented Oct 1, 2017

@liuggio any news?

@stof
Copy link

stof commented Oct 2, 2017

@liuggio why does it need a major version ? This changes a class in the testsuite fixtures. This class is not autoloadable when using the bundle in projects.

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