Skip to content

Conversation

dereuromark
Copy link
Member

@dereuromark dereuromark commented Aug 8, 2025

Implements some of the ideas of #822 as showcase

Resolves #475 by allowing to not use namespaces, but anonymous classes.

What do you think? Which ones do we want to pursue?

Open questions:

  • Should we change the location from config/Migrations/ to resources/Migrations/ or is it fine?
  • Do we want to default to the new style? while still allowing the old ones to work of course

By allowing the old ones to continue to work, we should provide reasonable BC here.

@dereuromark dereuromark marked this pull request as draft August 8, 2025 13:56
@LordSimal
Copy link
Contributor

LordSimal commented Aug 8, 2025

I dont understand why we need to wrap the anonymous class in a function.

cant we just get the version from the filename and thats it?

But I am all for anonymous classes for migrations

@dereuromark
Copy link
Member Author

Then you need to change

    public function __construct(int $version)

and setVersion() or alike on it

fair enough for me.

@jamisonbryant
Copy link
Contributor

Should we change the location from config/Migrations/ to resources/Migrations/ or is it fine?

I think it's fine the way it is. Unless we want to move Seeds also? There is currently nothing in the resources/ dir in the Cake app skeleton (5.x) which has likely led to users placing their own files and directories in there. If we move pre-existing directories in there all of a sudden it could be jarring.

Do we want to default to the new style? while still allowing the old ones to work of course

I think if it's configurable it shouldn't much matter. Users can set which style they prefer and forget about it. If we're going to do a major plugin release, we should default to the new version. Perhaps, if we decide to backport this behavior to a 4.x release, we can default to the legacy version.

We could also default to the legacy type and print a message "Baking migration using Legacy template, update 'Migrations.style' to try the new anonymous class style"

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks good to me 👏

@dereuromark
Copy link
Member Author

If we're going to do a major plugin release, we should default to the new version.

I agree.
Especially since then all CS issues should be resolved and it should be a clean code path as well (I can remove all my silencing for the config/Migrations folder then)

@dereuromark
Copy link
Member Author

Once this is done, we need to also tackle Seeds, as they have the same PSR2 namespace missing.

@dereuromark
Copy link
Member Author

dereuromark commented Aug 30, 2025

@markstory How do we best proceed here?

And we should probably squash merge to avoid extra conflicts in the future with other PRs.

@dereuromark dereuromark marked this pull request as ready for review August 30, 2025 00:49
@markstory
Copy link
Member

I think if it's configurable it shouldn't much matter. Users can set which style they prefer and forget about it. If we're going to do a major plugin release, we should default to the new version. Perhaps, if we decide to backport this behavior to a 4.x release, we can default to the legacy version.

We could also default to the legacy type and print a message "Baking migration using Legacy template, update 'Migrations.style' to try the new anonymous class style"

I think we can have 5.x switch to the newer format as long as we maintain backwards compatibility for 'old' migrations that applications might have. Existing migrations should continue to be applied and executed. Ideally with zero effort on the developers' part. It would also be good to have a Configure value (Migrations.style works for me) that enables 'old' migrations to be generated. This would let us cover the scenario of trialing a new migrations version and needing backwards compatibility with older migrations.

If folks want 5.x to only provide the anonymous style migrations, then we'll need to introduce forwards compatibility and tooling to convert migrations over in 4.x.

I think the getting developer experience smooth with this change is going to be important. I personally don't want maintenance toil from my database migrations.

@dereuromark
Copy link
Member Author

Right now both work equally.
Either way we should move forward.

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.

4 participants