cakephp / phinx

PHP Database Migrations for Everyone
https://phinx.org
MIT License
4.46k stars 890 forks source link

Migration classes do not comply with PSR-4 autoloading standard #1784

Open kaktusas2598 opened 4 years ago

kaktusas2598 commented 4 years ago

Hello,

After trying to run composer update (v 1.10.1), it started to complain about every single migration class not complying with PSR-4 autoloading standard and telling me it won't be autoloaded once we switch to composer v2.0.

All our migration classes reside in root/database/src/Migration and fall under namespace Pi\Database\Migration. As far as I can see, problem here is that each migration php file must be prefixed with timestamp and this timestamp is not used in class name and this is why it complains about PSR-4 standard.

I think one solution is would be to add timestamps to class names, are there any other ways to workaround this?

dereuromark commented 4 years ago

Not using psr4 namespacing here IMO.

In our scope of CakePHP Migrations as wrapper, we just have files in config/Migrations that do not have namespaces. They are just directly read and used by the system. So no autoload errors or composer problems.

E.g. config/Migrations/20190820233300_MigrationInit.php:

<?php
declare(strict_types=1);

use Migrations\AbstractMigration;

class MigrationInit extends AbstractMigration {}
MasterOdin commented 4 years ago

My view would be that generated migrations should be named V<version><OptionalClassName> and it would be PSR-4 compliant, and all new classes should sort after the "old style" ones. E.g, phinx create Foo would give you V20200512122342Foo.php and same class.

Perhaps land this in 0.13 along with making the argument to create optional (where the V was already getting pre-pended to the class name in that case).

dereuromark commented 4 years ago

This would be a breaking change we could think about for 0.13 What is the benefit of having psr4 compatibility over the current way of standalone class files?

MasterOdin commented 4 years ago

I think it's probably a combination of "best practices of PHP", "some people like running their migrations through their linter which enforces PSR-4", and "some people want to be able to refer to migrations through their autoloader". A related issue with people wanting something similar (or steps towards it) is #168, which should be done at the same time I think.

One option would be might be to implement a flag to the configuration file that disables PSR-4 style classes at the same time, since that shouldn't be too hard to support, as we'll probably have to maintain ability to load old files for some time anyway. At some point, might be good to move toward something like what was proposed in #1087 though.

dereuromark commented 4 years ago

I think we should probably aim to support one of those only, to keep things simple and complexity out of code. Better to stick to conventions which we can easily document and enforce. So going to those class names for 0.13 sounds reasonable if we can at least offer a good migration path for existing ones.

dereuromark commented 4 years ago

This also seems to ref https://github.com/cakephp/phinx/issues/1820

I also still always have to exclude migrations folder in sniffer:

FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------------------------------------
 6 | ERROR | Class name "MigrationLanguages" doesn't match filename, expected "20201014000218_MigrationLanguages"
 6 | ERROR | Each class must be in a namespace of at least one level (a top-level vendor name)
FLasH3r commented 3 years ago

Any updates on this? class names can't start with a number as @kaktusas2598 suggested