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

[10.x] Guess table name correctly in migrations if column's name have ('to', 'from' and/or 'in') terms #48437

Merged
merged 3 commits into from Sep 18, 2023

Conversation

i350
Copy link
Contributor

@i350 i350 commented Sep 18, 2023

This PR improves the table guesser, to work correctly if the user trying make migration that add/update/delete column have one of the terms ("to", "from" or/and "in")

For example, if the user executed the command

php artisan make:migration add_is_sent_to_3rd_party_service_column_to_users_table

The table name guesser before this update will return "3rd_party_service_column_to_users" as a table name, but after the update, it returns "users"

@i350 i350 changed the title [10.x] Guess table name correctly if column have "to" term [10.x] Guess table name correctly in migrations if column's name have ('to', 'from' and/or 'in') terms Sep 18, 2023
@i350 i350 marked this pull request as ready for review September 18, 2023 10:37
Comment on lines 12 to 15
const CHANGE_PATTERNS = [
'/_(to|from|in)_(\w+)_table$/',
'/_(to|from|in)_(\w+)$/',
'/_(to|from|in)_(?!.*?(\_to\_|\_in\_|\_from\_))(\w+)_table$/',
'/_(to|from|in)_(?!.*?(\_to\_|\_in\_|\_from\_))(\w+)$/',
];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just add .* in the beginning of the regex, so '/_(to|from|in)_(\w+)$/' becomes '/.*_(to|from|in)_(\w+)$/'. Based on my testing it also solves the issue and makes regex simpler.

Copy link
Contributor

Choose a reason for hiding this comment

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

But better double check it before applying, maybe I missed something :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Overall thanks for PR, I think it's useful for cases like you described.

Copy link
Contributor

Choose a reason for hiding this comment

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

@i350, good catch on the bug here. I agree with @hivokas though, this regex could be a little simpler, for example:

    const CHANGE_PATTERNS = [
        '/.+_(to|from|in)_(\w+)_table$/',
        '/.+_(to|from|in)_(\w+)$/',
    ];

Given the greedy nature of regular expressions, this will force it to only match the "last" instance in the string. I quickly tried it with your new test cases in regex101. Feel free to update your PR and see if it works for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @hivokas & @jasonmccreary

I just pushed an update to use the simplified regex you provided (52eac63)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your work @i350

@taylorotwell taylorotwell merged commit 8e319c0 into laravel:10.x Sep 18, 2023
20 checks passed
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.

None yet

4 participants