Skip to content

Conversation

@ckrack
Copy link
Contributor

@ckrack ckrack commented Sep 3, 2018

The make:crud controllers rely on ParamConverters,
which are provided by framework-extra-bundle.
Fixes #255

The make:crud controllers rely on ParamConverters,
which are provided by framework-extra-bundle.
Fixes #255
@weaverryan
Copy link
Member

Hey @ckrack!

Hmm, this makes sense. But, we have a test to catch this.... @sadikoff what do you think? It looks like our CRUD test definitely checks controllers that use this - https://github.com/symfony/maker-bundle/blob/master/tests/fixtures/MakeCrud/tests/GeneratedCrudControllerTest.php - so if we're missing a dependency, shouldn't that test fail?

@ckrack
Copy link
Contributor Author

ckrack commented Sep 5, 2018

Hi @weaverryan
I haven't found out all details about the tests, but:
There is an exclude for tests/fixtures in phpunit.xml.dist

I found one other test here, but it's only making a GET to the list method. https://github.com/symfony/maker-bundle/blob/master/tests/fixtures/MakeCrudRepository/tests/GeneratedCrudControllerTest.php

In https://github.com/symfony/maker-bundle/blob/master/tests/Maker/FunctionalTest.php, there are a couple of comments // need for crud web tests, maybe that's an indication of missing tests?

MakeCrud::configureDependencies only checks for the existance of Symfony\Component\Routing\Annotation\Route, which is part of the routing component.
If this fails, it suggests installing "annotations". I'd change this to suggest the router in this place and add the block in this pull request for the annotations.

By the way: I can't run the tests properly.
When i leave the example db DSN in phpunit.xml.dist, it complains about Access denied. When i replace it with sqlite, it complains about:

  1. Symfony\Bundle\MakerBundle\Tests\Maker\FunctionalTest::testCommands with data set "crud_with_no_base" (Symfony\Bundle\MakerBundle\Test\MakerTestDetails Object (...))
    Exception: Could not find "mysql://db_user:[email protected]:3306/db_name" inside "phpunit.xml.dist"

The Route now requires the router.
The ParamConverter requires annotations (framework-extra-bundle).
@weaverryan
Copy link
Member

Hey @ckrack!

Ah, this is great debugging! This issue is clear to me now. A few important things:

MakeCrud::configureDependencies only checks for the existance of Symfony\Component\Routing\Annotation\Route, which is part of the routing component.
If this fails, it suggests installing "annotations". I'd change this to suggest the router in this place and add the block in this pull request for the annotations.

This is the key. This bug is because we recently changed from using the Route class from FWExtraBundle to the core one.

The reason the tests aren't catching it because of another small bug in the tests. Specifically, in the "mini app's" setup by the tests, we install ALL the dependencies listed. So, annotations is being installed. But actually, we should only install any "missing" dependencies (the same way the user would - i.e. the user would never get a message currently to install annotations because they will always have the core Route class). I'm going to fix that in a separate pull request.

So yes, this is perfect. 💯 :)

@weaverryan weaverryan merged commit 6379141 into symfony:master Sep 6, 2018
weaverryan added a commit that referenced this pull request Sep 6, 2018
This PR was squashed before being merged into the 1.0-dev branch (closes #256).

Discussion
----------

Add framework-extra-bundle dependency

The make:crud controllers rely on ParamConverters,
which are provided by framework-extra-bundle.
Fixes #255

Commits
-------

6379141 Switch classes in MakeCrud::configureDependencies
39b5447 Add framework-extra-bundle dependency
@weaverryan
Copy link
Member

weaverryan commented Sep 6, 2018

Oh, one more thing:

By the way: I can't run the tests properly.
When i leave the example db DSN in phpunit.xml.dist, it complains about Access denied. When i replace it with sqlite, it complains about:
Symfony\Bundle\MakerBundle\Tests\Maker\FunctionalTest::testCommands with data set "crud_with_no_base" (Symfony\Bundle\MakerBundle\Test\MakerTestDetails Object (...))
Exception: Could not find "mysql://db_user:[email protected]:3306/db_name" inside "phpunit.xml.dist"

I need your help with this :). You are the 2nd person today who has reported this. So, I think there is a legitimate issue. However, I can't repeat it locally or on TravisCI. What does your tests/tmp/cache/flex_project/phpunit.xml.dist file look like? Also, what does this same phpunit.xml.dist file look like inside one of the maker_* directories inside that same tests/tmp/cache folder (e.g. what does tests/tmp/cache/maker_98f004a33a/phpunit.xml.dist)?

We create a mini Flex app during the tests. We run composer require phpunit, and that recipe should give us a phpunit.xml.dist file that contains the string in the error that (for some reason) cannot be found.

If you can help a bit debugging this, it would be awesome. The test suite is super powerful... but some people are having trouble running it, which is no good :).

Update: nevermind - I read your message too quickly :). Copy phpunit.xml.dist to phpunit.xml inside your MakerBundle, and change the TEST_DATABASE_DSN to a MySQL connection string (user/pass) that works for your computer. We default to "root" with no password. If you use a different user/pass on your machine, you need to customize that :).

weaverryan added a commit that referenced this pull request Sep 6, 2018
…lled (weaverryan)

This PR was squashed before being merged into the 1.0-dev branch (closes #260).

Discussion
----------

Fixing a bug in the tests where too many deps might be installed

This could create a situation where we tell a user to install libraries
A, B & C only, but really, we also depend on library D, which was
not recommended because we are missing that dependency from our Maker.
However, library D may have been installed in the functional test
(because we previously installed ALL dependencies, even if it was
related to a class we already had).

This would have caught #256.

Commits
-------

27787c4 Fixing a bug in the tests where too many deps might be installed
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.

2 participants