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

Move all files to src and tests #323

Closed
ruudk opened this issue Jun 1, 2017 · 14 comments
Closed

Move all files to src and tests #323

ruudk opened this issue Jun 1, 2017 · 14 comments

Comments

@ruudk
Copy link

ruudk commented Jun 1, 2017

Is it ok if I create a PR that moves all sources to src and the tests in tests? This way, the tests won't get distributed with composer.

I'm currently trying to migrate my Symfony 3.2 project to the Symfony 3.0 directory structure format and I'm getting weird errors like this:

$ composer install
...
...\Core\Composer\ScriptHandler::clearCache
PHP Fatal error:  Cannot declare class AppKernel, because the name is already in use in /vagrant/vendor/liip/functional-test-bundle/Tests/App/AppKernel.php on line 54

Fatal error: Cannot declare class AppKernel, because the name is already in use in /vagrant/vendor/liip/functional-test-bundle/Tests/App/AppKernel.php on line 54
Segmentation fault (core dumped)
Script ....\Core\Composer\ScriptHandler::clearCache handling the post-install-cmd event terminated with an exception


  [RuntimeException]
  An error occurred when executing the "'cache:clear --no-warmup'" command:
  Fatal error: Cannot declare class AppKernel, because the name is already in use in /vagrant/vendor/liip
  /functional-test-bundle/Tests/App/AppKernel.php on line 54
  PHP Fatal error:  Cannot declare class AppKernel, because the name is already in use in /vagrant/vendor
  /liip/functional-test-bundle/Tests/App/AppKernel.php on line 54
  Segmentation fault (core dumped)


install [--prefer-source] [--prefer-dist] [--dry-run] [--dev] [--no-dev] [--no-custom-installers] [--no-autoloader] [--no-scripts] [--no-progress] [--no-suggest] [-v|vv|vvv|--verbose] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--ignore-platform-reqs] [--] [<packages>]...
@alexislefebvre
Copy link
Collaborator

It will probably be simpler to exclude some directories from the archive: https://stackoverflow.com/questions/17049313/how-to-ignore-directories-with-composer

@ruudk
Copy link
Author

ruudk commented Jun 1, 2017

@alexislefebvre Thanks! I found that one as well, wasn't sure if it would work. Going to test it now in #325

@ruudk
Copy link
Author

ruudk commented Jun 1, 2017

@alexislefebvre It doesn't work. Still gives the same error. So I think it's better to just split it.

@alexislefebvre
Copy link
Collaborator

It will probably be simpler to exclude some directories from the archive: https://stackoverflow.com/questions/17049313/how-to-ignore-directories-with-composer

I tested it by creating a branch with a .gitattributes file on this repo: 0039e07

Then I installed this branch of the package:

$ composer require liip/functional-test-bundle:dev-add-dot-gitattributes-file

Unfortunately, the Tests/ directory is still here 😞:

$ ls vendor/liip/functional-test-bundle/Tests/
App/  AppConfig/  AppConfigLeanFramework/  AppConfigMysql/  AppConfigPhpcr/  Command/  DependencyInjection/  Test/

Maybe it's due to the fact that Composer cloned the repo instead of downloading a zip file. Maybe it works only when a tag is created.

@ruudk
Copy link
Author

ruudk commented Jun 15, 2017

Why not just move it into src and tests? :) #324

@alexislefebvre
Copy link
Collaborator

Because I'm trying to find a simpler solution before moving all the Tests/ files.

Can you please share the steps you followed in order to have the error you mentioned in the beginning of this issue?

@Aerendir
Copy link

Aerendir commented Jun 22, 2017

If I can, I agree with @ruudk : following the standards is a mandatory step to make a bundle understandable, apart from the eventual errors that may happen, especially if the work is already done.. (#324). IMHO.

@alexislefebvre
Copy link
Collaborator

@Aerendir

following the standards

It doesn't look like moving all the files to src/ is a standard. Not all bundles use this files structure.

@lsmith77 Can you please give your opinion about these changes? What do you think of moving all the files to src and tests directories? Thanks.

@Aerendir
Copy link

@alexislefebvre Beh, it's clearly a Symfony's standard at least (adopted from the Unix world): http://fabien.potencier.org/symfony4-directory-structure.html

I understand not all bundles adopt this new structure, but this is not a valid reason, in my opinion, to not adopt it for LiipFunctionalTestsBundle: if we can improve, we should...

I don't touch the code of this bundle since a lot of time, and reading again the code, in the first time confused me: where should have I to look for the classes I have to use in my project?

schermata 2017-06-27 alle 11 12 11

Sure, opening each folder solves the dilemma, but... I have to open both folders first. It's not immediate.

I think that starting to use a more robust folder structure helps also to make a deeper refactoring that will make the bundle better: #218

@alexislefebvre
Copy link
Collaborator

@Aerendir the link is about a good practice for a Symfony project (with var/, web/, etc.), not a Symfony bundle.

Anyway I agree with you @ruudk and @Aerendir that this is a common practice and we don't have any other option to avoid the bug repored in this issue. My only concern now is that it will break every PR. we have to check that this is worth the change.

@Aerendir
Copy link

Aerendir commented Jun 27, 2017

@alexislefebvre "Everything is a bundle in Symfony" 😝
No, seriously, I know it is about a generic project, but I think is anyway a good practice to separate the two things: in one folder the source code, in another the tests...

I'm happy you agree :)

@ruudk ruudk closed this as completed Aug 10, 2017
@Aerendir
Copy link

@ruudk Why did you close this issue?

@ruudk
Copy link
Author

ruudk commented Aug 10, 2017

Because I don't want to pursue with the PR anymore. I got it working without.

@phillipsnick
Copy link

Any details on how you got it working? Having the same problem with Cannot declare class AppKernel, because the name is already in use in and can't just seem to be going in circles.

I agree that src and tests is the best approach, so much easier to follow.

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

No branches or pull requests

4 participants