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

Validate datetime for version. #2249

Merged
merged 2 commits into from
Dec 26, 2023
Merged

Validate datetime for version. #2249

merged 2 commits into from
Dec 26, 2023

Conversation

dereuromark
Copy link
Member

@dereuromark dereuromark commented Dec 4, 2023

Resolves #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.

@dereuromark dereuromark added the bug label Dec 4, 2023
@dereuromark dereuromark added this to the 0.15.x milestone Dec 4, 2023
@MasterOdin
Copy link
Member

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.

Comment on lines 386 to 388
if (!$version || $length === 14) {
return;
}
Copy link
Member

@MasterOdin MasterOdin Dec 20, 2023

Choose a reason for hiding this comment

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

A follow-up here would be to validating that the various pieces of the string are all valid, e.g. that don't have 00 or 13 for MM, potentially including also complexity in not allowing invalid dates (e.g. no 30 for MM == 02).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but then it wouldnt fit in a "minor" release probably as people could have existing invalid ones that are valid in range (sorting).
So for now I would like to keep it as is, but in the next major we could try to limit it to a more sane value in general.

@dereuromark
Copy link
Member Author

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

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

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.

@MasterOdin MasterOdin merged commit 91a4240 into 0.x Dec 26, 2023
12 checks passed
@MasterOdin MasterOdin deleted the 0.x-validation branch December 26, 2023 23:11
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.

Validate version int range
2 participants