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

[6.x] Fix migration class duplicate check #30095

Merged
merged 2 commits into from Sep 25, 2019

Conversation

@ROOKIE20570
Copy link
Contributor

commented Sep 25, 2019

Closes #30062.

@ROOKIE20570 ROOKIE20570 force-pushed the ROOKIE20570:6.x branch from d4f4b65 to 064964d Sep 25, 2019
@sisve

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

This changes a method signature and may break existing code. Should it target another Laravel version?

@ROOKIE20570

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

This changes a method signature and may break existing code. Should it target another Laravel version?

I am wondering if I can add another method to verify that it already exists, but that is not in line with the DRY principle.Let me think about it

@ROOKIE20570 ROOKIE20570 reopened this Sep 25, 2019
@ROOKIE20570 ROOKIE20570 force-pushed the ROOKIE20570:6.x branch from 5026a67 to a80ec3f Sep 25, 2019
@ROOKIE20570 ROOKIE20570 force-pushed the ROOKIE20570:6.x branch from a80ec3f to adb1e8d Sep 25, 2019
@ROOKIE20570

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

This changes a method signature and may break existing code. Should it target another Laravel version?

I added a default parameter, so the code that depends on this method can run

@gocanto

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

Nice work ⭐️

I added a default parameter, so the code that depends on this method can run

It might bring issues if somebody is extending this class since the given method is marked as protected.

You probably can extract the common path logic check from there, so you don't duplicate yourself. Therefore, this change can still be applied without breaking the API.

Nevertheless, you might want to wait for official feedback to keep working on it.

@GrahamCampbell GrahamCampbell changed the title fix migration class duplicate check [6.x] Fix migration class duplicate check Sep 25, 2019
@taylorotwell taylorotwell merged commit 1a7e96d into laravel:6.x Sep 25, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/styleci/pr The analysis has passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

While this does add an argument to a protected method, the likelihood of any given Laravel application extending this specific method seems pretty negligible to me.

@driesvints

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

This broke the build. @ROOKIE20570 please make sure that the tests pass next time, thanks.

@ROOKIE20570

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

This broke the build. @ROOKIE20570 please make sure that the tests pass next time, thanks.

Sorry, I was not familiar with it before, I will definitely do better next time.

@driesvints

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

@ROOKIE20570 no worries at all 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.