-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Updated the application to use Symfony Flex #617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers 8000 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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great 🎉!
config/packages/app.yaml
Outdated
@@ -0,0 +1,25 @@ | |||
services: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this file can be removed in favor of config/services.yaml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10000Yes, I removed it ... but whenever you run composer install
, the file is recreated, so I prefer to keep it until Symfony removes it officially.
@@ -0,0 +1,7 @@ | |||
twig: | |||
paths: ['%kernel.project_dir%/templates'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As soon as symfony/symfony#23339 to be merged, the default path to override bundles will be templates/bundles/
, so what about omit (i.e. for templates) the src/Resources/
path to override the TwigBundle
's templates? this configuration should be enough to move them:
twig:
paths:
templates: ~
templates/bundles/Twig: Twig
Later, the error template can live there templates/bundles/Twig/Exception/error.html.twig
. WDYT?
.env.dist
Outdated
# Format described at http://docs.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/configuration.html#connecting-using-a-url | ||
# For a MySQL database, use: "mysql://root@127.0.0.1:3306/symfony?charset=utf8mb4&serverVersion=5.7" | ||
# Set "serverVersion" to your server version to avoid edge-case exceptions and extra database calls | ||
DATABASE_URL="sqlite:///%kernel.project_dir%/var/data.db" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meanwhile... we could use relative paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With kernel.project_dir
it's much readable I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the kernel.project_dir
param is the right thing to do, but it doesn't work yet. Relative paths should be OK too, but I couldn't make it work. Only absolute paths worked for me. But probably (hopefully) I'm doing something wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javiereguiluz, There is no data.db
file. I think it should be sqlite:///%kernel.project_dir%/var/data/blog.sqlite
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, thanks! But it doesn't work either. Only absolute paths work for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javiereguiluz, It doesn't work because the current working dir is public
. You should use this: sqlite:///../var/data/blog.sqlite
. What about to change the current working dir in the front controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the current working dir is different when we're executing a command and when we're accessing through the browser, so relative path shouldn't work.
handler_id: session.handler.native_file | ||
save_path: '%kernel.project_dir%/var/sessions/%kernel.environment%' | ||
|
||
trusted_hosts: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't this option deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's trusted_proxies
the one that's deprecated: https://symfony.com/blog/fixing-the-trusted-proxies-configuration-for-symfony-3-3
.env.dist
Outdated
###> symfony/swiftmailer-bundle ### | ||
# For Gmail as a transport, use: "gmail://username:password@localhost" | ||
# To disable sending emails, use: "null://localhost" | ||
MAILER_URL=smtp://localhost:25?encryption=tls&auth_mode=login |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javiereguiluz, you should quote the variable value with a single quotes. IMHO, it's better to use single quotes instead of double ones in the environment files, because double quotes don't support variable expansion and etc.
.env.dist
Outdated
# Format described at http://docs.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/configuration.html#connecting-using-a-url | ||
# For a MySQL database, use: "mysql://root@127.0.0.1:3306/symfony?charset=utf8mb4&serverVersion=5.7" | ||
# Set "serverVersion" to your server version to avoid edge-case exceptions and extra database calls | ||
DATABASE_URL="sqlite:///%kernel.project_dir%/var/data/blog.sqlite" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @yceruto, it's better to use relative path here. We should avoid to use container parameters in the environment files, because it makes them unshareable with others applications.
For example, I want to create bash script to back up the database:
#!/usr/bin/env bash
source .env;
mysqldump "$DATABASE_URL"; # It won't work because variable contains the %kernel.project_dir% parameter
@javiereguiluz what exactly are the problems with |
result of executing: $ composer require --dev dama/doctrine-test-bundleUsing version ^3.0 for dama/doctrine-test-bundle Problem 1 Installation failed, reverting ./composer.json to its original content. |
@javiereguiluz fixed with the latest
|
@dmaicher thanks! Now I can install it and I've added back the listener in PHPUnit config ... but it doesn't seem to be working. The first test is OK, but the second one is not, so the database is not restored:
|
@javiereguiluz and is the bundle is also loaded like it was done before here? https://github.com/symfony/symfony-demo/pull/617/files#diff-c23da96dc8986a4cee89980f3aad30e0L54 |
@dmaicher I'm an idiot! Added the bundle as |
Typical mistakes of every symfony user lol |
Tests are failing on Travis:
|
@javiereguiluz weird fail 😢 Maybe try to add |
Actually I don't see the command |
composer.json
Outdated
"symfony/swiftmailer-bundle": "^3.0", | ||
"symfony/translation": "^3.3", | ||
"symfony/validator": "^3.3", | ||
"symfony/var-dumper": "^3.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put var-dumper to dev requirements?
composer.json
Outdated
"App\\Tests\\": "tests/" | ||
}, | ||
"files": [ | ||
"vendor/symfony/var-dumper/Resources/functions/dump.php" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this file can be removed here, because var-dumper
is now required directly.
# into clickable links that open the given file using your favorite IDE. | ||
# When 'ide' is set to null the file is opened in your web browser. | ||
# See https://symfony.com/doc/current/reference/configuration/framework.html#ide | ||
ide: ~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file uses null
and ~
. Imo we should only use one of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need help. Travis is failing with this error:
But the |
config/packages/security.yaml
Outdated
# To load users from somewhere else: https://symfony.com/doc/current/security/custom_provider.html | ||
database_users: | ||
entity: { class: AppBundle:User, property: username } | ||
entity: { class: App:User, property: username } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should use App\Entity\User there?
@javiereguiluz this file of the sec-checker recipe is missing: https://github.com/symfony/recipes/blob/master/sensiolabs/security-checker/4.0/config/packages/dev/security_checker.yaml#L5 |
🤦♂️ @gharlan that was it! Thank you. |
... and a million changes later, the Symfony Demo is upgraded to Symfony Flex and tests are green 🎉 🎉 🎉 Thank you all for your help! Now, please tell me more things to improve before merge. Thanks! |
.php_cs.dist
Outdated
@@ -11,12 +11,12 @@ COMMENT; | |||
|
|||
$finder = PhpCsFixer\Finder::create() | |||
->in(__DIR__) | |||
->exclude('config') | |||
->exclude('var') | |||
->exclude('web/bundles') | |||
->exclude('web/css') | |||
->exclude('web/fonts') | |||
->exclude('web/js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web -> public
composer.json
Outdated
"file": "app/config/parameters.yml" | ||
"symfony": { | ||
"id": "01BP22MQ00QH5WD6NN6HHD3EX4", | ||
"allow-contrib": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe set allow-contrib to true
?
So if people use the demo and try to add a package/bundle which is available in contrib recipes, it will work just fine?
Just an idea, I'm not sure.
config/services.yaml
Outdated
# classes (e.g. container parameters) so you must define those arguments explicitly | ||
AppBundle\Command\ListUsersCommand: | ||
App\Command\ListUsersCommand: | ||
arguments: | ||
$emailSender: '%app.notifications.email_sender%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove arguments:
here
->exclude('public/bundles') | ||
->exclude('public/css') | ||
->exclude('public/fonts') | ||
->exclude('public/js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public/css
, public/fonts
and public/js
=> public/build
I could be wrong but I think the fixtures might need looking at. Have just cloned your fork and when attempting to load the fixtures I get the following:
Think its due to the lack of a Bundle. Edit: I've just now, noticed your comment ( @javiereguiluz ) on their PR to fix this. - doctrine/DoctrineFixturesBundle#192 I can confirm the fix provided in that PR does solve the issue. |
composer.json
Outdated
"symfony/polyfill-apcu": "^1.4", | ||
"symfony/security-bundle": "^3.3", | ||
"symfony/swiftmailer-bundle": "^3.0", | ||
"symfony/templating": "^3.3", |
There was a problem hiding this comment.
Choose a reason for hiding this commen 10000 t
The reason will be displayed to describe this comment to others. Learn more.
you can remove the templating component (revert e8d0b8a), since the error is fixed in 3.3.7.
This fixes #616.
The application works, but I need to check tests because there were some issues with the
dama/doctrine-test-bundle
dependency used to run tests.When testing this locally, in your .env file, you must use an absolute file path to define the SQLite path. Otherwise you'll see this error:
This is a known limitation of DotEnv and we're working on it.