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

Update config example to match flex recipe. #523

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

Conversation

yivi
Copy link

@yivi yivi commented Jan 3, 2024

Update config example to match the flex recipe, since the included example ("App\Migrations") went directly against it.

Update config example to match the flex recipe, since the included example ("App\Migrations") went directly against it.
@greg0ire
Copy link
Member

greg0ire commented Jan 3, 2024

Can you please post a link to the PR that changed the flex recipe?

@yivi
Copy link
Author

yivi commented Jan 3, 2024

I'll look for it.

I'm only aware because of this because today cloned a new project today, and realized the docs didn't match the recipe advice.

'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?

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants