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

[5.7] Allow migration table name to be guessed without _table suffix #26429

Conversation

DivineOmega
Copy link
Contributor

@DivineOmega DivineOmega commented Nov 7, 2018

This PR modifies the TableGuesser class to allow the table name to be parsed from migrations that do not have the _table suffix.

This will allow devs to create migrations with shorter names and still have them pre-populated with the relevant creation or change boilerplate content. See the following examples.

# This longer migration name:
php artisan make:migration create_users_table

# Can now be just:
php artisan make:migration create_users

# And, this longer migration name:
php artisan make:migration add_status_column_to_users_table

# Can now be just:
php artisan make:migration add_status_column_to_users

Original functionality (longer migration names) still works fine. The original regex pattern is checked first, prior to checking the new regex pattern.

Even though this is a minor change, it should help speed up and make migration creation just a tiny bit more intuitive. This change was actually inspired by a colleague who recently accidentally missed out the _table prefix from their artisan command and asked me why the boilerplate content was not added.

This PR also adds a test which was missing from the TableGuesser class tests, to ensure migrations in created in the format foo_in_bar_table are parsed correctly.

@staudenmeir
Copy link
Contributor

staudenmeir commented Nov 7, 2018

Can't we just change the patterns to /^create_(\w+?)(_table)?$/ and /_(to|from|in)_(\w+?)(_table)?/?

@DivineOmega
Copy link
Contributor Author

@staudenmeir Thanks for the feedback.

I did it the way I have to ensure and make it very clear that the _table pattern was prioritised. If people feel this is redundant, I can certainly make that change.

@dextermb
Copy link

dextermb commented Nov 8, 2018

Can't we just change the patterns to /^create_(\w+?)(_table)?$ and /_(to|from|in)_(\w+?)(_table)??

I think you mean:

/^create_(\w+?)(_table)?$/

(You missed your end slashes)

@staudenmeir Thanks for the feedback.

I did it the way I have to ensure and make it very clear that the _table pattern was prioritised. If people feel this is redundant, I can certainly make that change.

I do like the idea of splitting them up on purpose to show that it should be _table, but I also like how clean it would be as a single regex.

It is worth noting that optimization is not as important as this is a single time operation.

@taylorotwell taylorotwell merged commit 4528502 into laravel:5.7 Nov 8, 2018
@DivineOmega DivineOmega deleted the feature/do-not-require-table-keyword-for-table-guesser branch November 8, 2018 22:20
@DivineOmega
Copy link
Contributor Author

@taylorotwell Thanks for merging. 🙂 👍

@vlakoff
Copy link
Contributor

vlakoff commented Nov 9, 2018

With current code, order of the patterns is important, i.e. if /^create_(\w+)$/ were first, the captured name would be users_table instead of expected users.

It seems this possibility of future regression is already covered by tests.

Still, how about adding a negative assertion? → /^create_(\w+)(?<!_table)$/
(note the working assertion is ?<!, as there is still match if using ?!)

@vlakoff
Copy link
Contributor

vlakoff commented Nov 9, 2018

This being said, I'd favor the single pattern /^create_(\w+?)(_table)?$/ too. Because it's actually safest than the current code (per my explanations just above), and simpler than my alternative.

@robclancy
Copy link
Contributor

Why in the world was this added? Such a terrible feature. Migrations aren't only for tables. I have never benefited from this feature (I will use the proper options like every command interface ever). But half my migrations are migrating data or refactors from one thing to another and added stuff I have to delete for no reason.

I don't know how anyone could think this was a good idea, do you work on projects where you make 5 tables then you're done and you couldn't type --table= like a real CLI would do?

@devcircus
Copy link
Contributor

About half the PRs for the framework now are for taking something you can do with 8 keystrokes and making it so you can do it with 5 instead. Basically just muddying the core or some like this where it actually has unintended side effects.

@robclancy
Copy link
Contributor

The feature is literally called a Guesser... I don't want to guess what code I want.

A super common thing to do would be create_super_admin and now that's going to try make a super_admin table? wtf

@robclancy
Copy link
Contributor

robclancy commented Mar 5, 2019

create_super_admin
change_users_status_to_enum_column this literally edits the table called enum_column 🤦‍♂️
revert_status_back_to_text text table lmao

Real migrations I have made that now this feature assualts.

create_tasks
add_assign_task_to_plan_content
refactor_all_revise_qa_tasks_to_same_one
rename_tasks_start_date_to_due_date
update_draft_fields_and_old_drafts_to_outdated
fix_client_transactions_table

And that's only looking at the last 50 migrations. But main thing is this is a CLI command changing how the command works based on the user input name.

Since I got this update half of the migrations I have made have done this and I thought there was a bug.

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.

7 participants