search

cakephp / phinx

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

Validate datetime for version. #2249

Closed dereuromark closed 8 months ago

dereuromark commented 8 months ago

Resolves https://github.com/cakephp/phinx/issues/2248

For legacy reasons and BC on the test harnesses I allowed the 0 to pass for now as well. I think thats fair since most mistakes would happen with actual datetime values.

MasterOdin commented 8 months ago

I've debated on if the appropriate place to put this is as a runtime check within initialization the migration, vs within the getMigrations method in which the migrations are initialized (and which all other methods use to get the list of migrations). I'm somewhat leaning towards the latter with the concept that if some alternative versioning scheme was desired, it'd probably be easier to support via the getMigrations method where we have access to the config object to determine said alternative versioning strategy.

Thoughts?

For legacy reasons and BC on the test harnesses I allowed the 0 to pass for now as well.

I'm curious what legacy reason beyond test harnesses exist to support 0 as a version. I feel like having support for this just for tests seems wrong, where the tests should be updated as it's maybe a bug for production to allow this?

I do like the approach here of only validating that the version integer is a valid date string, and not putting limits on what the allowed range of dates is, as I think that has the potential to be limiting for less value.

dereuromark commented 8 months ago

where the tests should be updated as it's maybe a bug for production to allow this?

could be a follow up if you want to further narrow the possible valid values.

dereuromark commented 8 months ago

I pushed the update, was a quick write-up. Hopefully we can get this in now asap to avoid people running into this like me and others did.

MasterOdin commented 8 months ago

Easy enough to move this check to somewhere else if we decide we need to down the road without breaking BC I think.

I do think this check does break BC though in that it's possible to bypass the date version naming scheme that phinx uses, and just use any number you wanted (e.g. having a counter), though you'd just have to manually create each migration. Not sure if anyone is doing that, so probably fine to just merge it into the next 0.15.x release, and easy to revert if there's enough pushback, or minimally just have them override the base AbstractMigration class.