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

if the migration class names are longer than 33 characters, running migr... #67

Closed
wants to merge 1 commit into from

Conversation

simkimsia
Copy link

...ations up or all will always prompt running migrated migrations

This also fixes the issue I raised in #66
#66

…igrations up or all will always prompt running migrated migrations
@simkimsia
Copy link
Author

@lorenzo Do take a look at this pull request. If there is a better way to fix the issue, I will gladly close this pull request and adopt the better way.

Thank you.

@burzum
Copy link
Contributor

burzum commented May 22, 2012

This is more a workaround to the problem than a real fix.

The issue is that the schema_migrations just takes a limited amount of chars, that needs to be increased but this causes other issues. We're discussing this.

@simkimsia
Copy link
Author

I agree with you @burzum that this is a workaround.

I made similar comments in #64.

Better a workaround now than nothing for those who rely on migrations plugin.

@burzum
Copy link
Contributor

burzum commented May 22, 2012

@simkimsia I'm busy right now with other work and can't do it, I might do it this week, if you want to do, do it and request a pull:

By a quick look and short internal discussion the proper fix to this would be to add a 2nd migration to the migrations plugin itself and make the type field varchar 128 or 255 and on top of that adding a check for the length when the class is generated and display user friendly error message on the console instead simply showing the exception / fatal.

@simkimsia
Copy link
Author

I agree with you @burzum

I will patiently wait because right now the workaround is good enough for me.

I want to add that because there is still an odd chance the classname might exceed 255 characters or whatever new limit you set, I prefer that you choose TEXT for the database field.

@burzum
Copy link
Contributor

burzum commented May 25, 2012

To be honest I think 128 are already far more than enough, these are already just 97 chars:

this_is_a_very_long_and_horrible_to_read_class_name_story_do_you_really want_it_to_be_that_long?

You should give it a short and descriptive name, not write a novel into the classname.

@simkimsia
Copy link
Author

True. Never really thought of that.

Then you are right.

Need to add exceptions for users putting in long names for migration files.
On May 25, 2012 6:41 PM, "Florian Krämer" <
reply@reply.github.com>
wrote:

To be honest I think 128 are already far more than enoug, these are
already just 97 chars:

this_is_a_very_long_and_horrible_to_read_class_name_story_do_you_really
want_it_to_be_that_long?

You should give it a short and descriptive name, not write a novel into
the classname.


Reply to this email directly or view it on GitHub:
#67 (comment)

@airdrummingfool
Copy link
Contributor

@burzum @simkimsia see my pull request #68. I implemented a migration file to change the size of the field and added a friendly error message in the console.

@burzum
Copy link
Contributor

burzum commented Jun 4, 2012

Merged it, thank you! :)

@burzum burzum closed this Jun 4, 2012
@simkimsia
Copy link
Author

Thank you @airdrummingfool

deizel pushed a commit to deizel/cakedc-migrations that referenced this pull request Mar 4, 2016
Update phinx version in composer.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants