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.

Sign up for GitHub

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

Jump to bottom

Update config example to match flex recipe. #523

Open
wants to merge 1 commit into
base: 3.3.x
Choose a base branch
from
Open

Update config example to match flex recipe. #523

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions Resources/doc/index.rst
View file
Open in desktop
Expand Up @@ -41,12 +41,11 @@ application:

# config/packages/doctrine_migrations.yaml

doctrine_migrations:
Copy link
Member

Choose a reason for hiding this comment

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

🤔 why are you removing this? It still appears here: https://github.com/symfony/recipes/blob/0d06144589aeb8eff11025d398f724f9545fab2c/doctrine/doctrine-migrations-bundle/3.1/config/packages/doctrine_migrations.yaml#L1

# List of namespace/path pairs to search for migrations, at least one required
migrations_paths:
'App\Migrations': '%kernel.project_dir%/src/App'
'AnotherApp\Migrations': '/path/to/other/migrations'
'SomeBundle\Migrations': '@SomeBundle/Migrations'
# namespace is arbitrary but should be different from App\Migrations
# as migrations classes should NOT be autoloaded
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what this means?

Copy link
Author

Choose a reason for hiding this comment

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

As I understand it, that migrations should not be autoloaded by the Symfony service container build process.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm isn't what you're thinking of called auto discovery, or do you really mean PHP autoloading ?

Copy link
Author

Choose a reason for hiding this comment

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

No, I didn't mean autoloading in the sense of "class autoloading", as provided by the composer autoloader. I meant being loaded and configured by the dependency injection component.

In any case, not using an "App" namespace would likely prevent both things in most scenarios targeted by the docs, so not sure if the difference is of actual relevance.

But frankly, I just meant to duplicate what the recipe said, since the docs seemed to me to be deferring to the recipe; and only including a default config for cases where Flex wasn't installed, and the recipe wasn't applied.

I actually didn't really think through if the recipe was actually right and the docs wrong, or vice versa. Maybe it's better I close my PR and let someone more knowledgeable or with more time to research to help with this.

Sorry I can't be of more help.

Copy link
Member

Choose a reason for hiding this comment

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

I was asking just in case you knew, or had the time to research, but I think I can find out.

Following the last link you provided, a few extra clicks lead to symfony/recipes#172

A bit more clicking leads to symfony/recipes#343 (comment), which corroborates that it really is PHP autoloading that is meant here. I'm not saying I understand why autoloading a migration class would break the mechanism that loads migration classes from a directory, but I think PHP autoloading is really what was meant here. Maybe @stof can help with this?

'DoctrineMigrations': '%kernel.project_dir%/migrations'

# List of additional migration classes to be loaded, optional
migrations:
Expand Down