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

Doctrine Migrations set up violates PSR-4 #343

Closed
dbu opened this issue Jan 15, 2018 · 12 comments
Closed

Doctrine Migrations set up violates PSR-4 #343

dbu opened this issue Jan 15, 2018 · 12 comments

Comments

@dbu
Copy link

dbu commented Jan 15, 2018

In a new flex project, i added migrations and generated a migration. The dir_name is set to src/Migrations, but the migrations are created with namespace DoctrineMigrations. The code style fixer wants to change that to App\Migrations. The migrate command finds the migration file, but then tries to instantiate the class with the DoctrineMigrations namespace. The migration is also not runtime code but more of a configuration thing. I changed the configuration to place the migration in config/Migrations (without setting up autoloading) to make things work.

I am not sure what the best solution is, but placing migrations in src/ but with a totally wrong namespace (no App prefix, folder Migrations vs namespace DoctrineMigrations) is counter-intuitive and leading to confusion.

@Pierstoval
Copy link
Contributor

I also think putting migrations in src is not the best option, but there was a lot of issues about migrations, like #112, #153, #172, #253

I hope some change is coming, because it was opinionated to src 🤔

@fbourigault
Copy link
Contributor

To handle such case, I use the following autoload section:

"autoload": {
    "psr-4": {
        "App\\": "src/"
    },
    "exclude-from-classmap": [
        "/src/Migrations/"
    ]
}

@stof
Copy link
Member

stof commented May 15, 2018

That's done on purpose, because doctrine/migrations does not rely on autoloading at all either (and I think that having any migration class autoloaded before the library runs the command might even break its location of available migrations)

@ghost
Copy link

ghost commented May 30, 2018

The bundle documentation mentions nothing about changing the namespace. Without @fbourigault's changes this recipe won't work well out of the box.

@dkarlovi
Copy link

dkarlovi commented Jun 5, 2018

I do the same thing as @fbourigault, with this:

doctrine_migrations:
    dir_name: "%kernel.project_dir%/src/App/Infrastructure/Doctrine/Migration/ORM"
    namespace: App\Infrastructure\Doctrine\Migration\ORM

@stof
Copy link
Member

stof commented Jun 5, 2018

@jrobeson the recipe works fine out of the box, because doctrine/migrations does not expect migration classes to be autoloadable (quite the opposite)

@dbu
Copy link
Author

dbu commented Jun 5, 2018

yep, it works. but it violates PSR by placing files in src/ with a wrong namespace.

@ChangePlaces
Copy link

As a codebase that seems to follow so many formal guidelines and best practices, it seems bizarre that it's turning its eye to this issue.

@fabpot
Copy link
Member

fabpot commented Aug 28, 2018

Closing as this is related to Doctrine migration, not Symfony. Nothing we can do here.

@fabpot fabpot closed this as completed Aug 28, 2018
@mrsax
Copy link

mrsax commented Nov 20, 2019

To handle such case, I use the following autoload section:

"autoload": {
    "psr-4": {
        "App\\": "src/"
    },
    "exclude-from-classmap": [
        "/src/Migrations/"
    ]
}

The problem is that one will not be able to make migrations anymore...

Better is to exclude it in the service.yaml so one doesn't get problems with tests etc either
services.yaml

App\:
    resource: '../src/*'
    exclude: '../src/{DependencyInjection,Entity,Migrations,Tests,Kernel.php}'

add the "exclude" line and you're set and migrations still work and composer.json still calls for PSR-4 as it should.

@adrianrudnik
Copy link

Just to make sure, this recipe and the use of migrations with their weird namespaces prevents any use of composer dump-autoload --classmap-authoritative and the optimizations that come with that correct?

@stof
Copy link
Member

stof commented Nov 4, 2020

@adrianrudnik no it does not. the whole point of this is that migrations classes must not be autoloaded. the doctrine/migrations library loads these classes in a special way, and letting the autoloader load them would mess with that logic.

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

No branches or pull requests

9 participants