search

cakephp / migrations

CakePHP database migrations plugin
Other
137 stars 116 forks source link

Avoid timestamp collisions on bake. #773

Closed dereuromark closed 5 days ago

dereuromark commented 6 days ago

Resolves https://github.com/cakephp/migrations/issues/772

screenshot

LordSimal commented 6 days ago

This is BC breaking. Anyone who has already overwritten those classes and added their own arguments will have to adjust their implementation.

dereuromark commented 6 days ago

Only for those that overwrote the Migration plugin command here, and also exactly that method with a different param. This is very unlikely to happen, and the resolution of this bug trumps this IMO. We can release a minor here with the changelog to clarify this.

markstory commented 6 days ago

This is BC breaking. Anyone who has already overwritten those classes and added their own arguments will have to adjust their implementation.

To me the 'public' API of migrations is the migrations classes and the CLI interface. While this would be a breaking change in the core framework, I don't think similar rules have to apply here.

LordSimal commented 5 days ago

I could definitely think of users who overwrote the BakeSimpleMigrationCommand or any commands based on it to have a custom filenaming schema.

Sure, it ain't everyone out there, but it goes against semantic versioning which we religiously comply with (at least with the major changes I tried to do in the past).

Not that I am against trying to fix this bug and maybe cause a few hickups for a few users, it's just inconsistent imho. So I'm fine with merging it if you guys are.

dereuromark commented 5 days ago

In the end it should be easy enough to adjust to this minor release. If you have an alternative solution, that could also work, of course. The main issue is to have the access to the path inside the method.

LordSimal commented 5 days ago

The state could be shared via a property instead of an additional argument. Then its just the currentTimestamp overwrite which could cause problems.

dereuromark commented 5 days ago

I made a PR to this one that should provide BC. Feel free to merge if CI passes (squash merge).