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

Avoid timestamp collisions on bake. #773

Merged
merged 3 commits into from
Nov 28, 2024
Merged

Conversation

dereuromark
Copy link
Member

@dereuromark dereuromark commented Nov 26, 2024

Resolves #772

screenshot

@dereuromark dereuromark added this to the 4.x (CakePHP 5) milestone Nov 26, 2024
@LordSimal
Copy link
Contributor

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

@dereuromark
Copy link
Member Author

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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

@dereuromark dereuromark mentioned this pull request Nov 27, 2024
@dereuromark
Copy link
Member Author

dereuromark commented Nov 27, 2024

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

* Fix BC topic.

* Fix BC topic.
@dereuromark dereuromark merged commit 89b6331 into 4.x Nov 28, 2024
16 checks passed
@dereuromark dereuromark deleted the 4.x-collision-avoidance branch November 28, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collision detect avoidance
3 participants