-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Symfony 4 directory structure #9643
Conversation
659da4e
to
9351e1b
Compare
3f99ed6
to
14e6221
Compare
.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 sqlite database, use: "sqlite:///%kernel.project_dir%/var/data.db" | ||
# Set "serverVersion" to your server version to avoid edge-case exceptions and extra database calls | ||
DATABASE_URL=mysql://root@127.0.0.1/sylius_${APP_ENV}?serverVersion=5.5 |
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.
serverVersion
should not be set here. See https://github.com/symfony/recipes/blob/cb6ea10b7bb60c4bd9c39ca3e92fb53e7ccce7e5/doctrine/doctrine-bundle/1.6/config/packages/doctrine.yaml#L12
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.
composer.json
Outdated
@@ -191,7 +185,7 @@ | |||
"psr-4": { | |||
"Sylius\\Tests\\": "tests/" | |||
}, | |||
"classmap": ["app/AppKernel.php", "app/AppCache.php", "app/TestAppKernel.php"] | |||
"classmap": ["src/Kernel.php"] | |||
}, | |||
"config": { | |||
"bin-dir": "bin" |
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 should be removed.
composer.json
Outdated
@@ -203,9 +197,6 @@ | |||
"symfony-web-dir": "web", |
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 should be removed.
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.
It'll be removed after adding Symfony Flex and removing SensioDistributionBundle in the next PR, I'll change it to public
before that.
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.
But why in a separate PR?
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 one is already +500 -800, the diff is getting unreadable and I want to make tests passing before proceeding further. It's master
so there's still over one and a half month until the next release.
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.
Nevertheless, Symfony 4 config structure and Symfony Flex are two different changes, that could go separately.
c722d00
to
d794fb2
Compare
87ef5ee
to
27071b9
Compare
@@ -23,6 +23,9 @@ imports: | |||
- { resource: "@SyliusCoreBundle/Resources/config/app/state_machine.yml" } | |||
- { resource: "@SyliusCoreBundle/Resources/config/app/fixtures.yml" } | |||
|
|||
parameters: | |||
sylius_core.public_dir: "%kernel.project_dir%/web" |
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 is of course wrong. But also, is it really necessary for this to be configurable?
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 is introduced only to make the changes backwards compatible - so that it won't fail if you upgrade from 1.2 to 1.3. It is overwritten in config/_sylius.yaml
to %kernel.project_dir%/public
.
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.
Actually, I wish we could take this opportunity to remove this file altogether...
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.
The relevant parts should be moved into the config.yml of Sylius-Standard and of the test app, respectively.
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.
Introducing more config files here is only gonna create a bigger problem than we already have.
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.
What do you mean by introducing more config files? config/packages/_sylius.yaml
follows the Symfony 4 standard of having config files for each package. We can't remove the config files in bundles in 1.x
branch. As for configs in Sylius-Standard and PluginSkeleton and their synchronisation, this is a whole different issue to be handled and I believe doing it all at once (Symfony 4 directory structure, Symfony Flex, config files synchronisation) would result in not having anything that works (and if it does, it would probably result in some nasty BC breaks).
Merging it for now, if there's any change needed, we can still revert it or customise before v1.3.0 release in late September. |
This PR switches Sylius to Symfony 4 directory structure. The PR introducing Symfony Flex in Sylius will follow shortly 🎉