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

Arguments fix #51

Merged
merged 2 commits into from
Mar 2, 2022
Merged

Arguments fix #51

merged 2 commits into from
Mar 2, 2022

Conversation

thyseus
Copy link
Collaborator

@thyseus thyseus commented Feb 18, 2022

Assume we have a command:

        protected $signature = 'group:backup {server}';

and configure laravel-database-schedule to run, currently it runs:

php artisan group:backup server='my-server'

which is wrong; it should run:

php artisan group:backup my-server

This MR fixes that problem.

@thyseus thyseus force-pushed the arguments-fix branch 2 times, most recently from e7948c7 to ee9da98 Compare February 18, 2022 17:35
@thyseus
Copy link
Collaborator Author

thyseus commented Feb 18, 2022

@robersonfaria i am struggling with fixing this test:

https://github.com/robersonfaria/laravel-database-schedule/runs/5251370512

do you have an idea what is going on ?

@robersonfaria
Copy link
Owner

It's exactly the point where you changed, before you received server='my-server', now you only receive the value, in the test you have to test the same.

In the tests/Unit/ScheduleCommandsTest.php file on the line 67 you need to change to:
->with('inspire', ['1'])

so it should work

@thyseus
Copy link
Collaborator Author

thyseus commented Feb 18, 2022

@robersonfaria thanks a lot, that was easy. Do you agree in general that this is the better expected behavior of laravel-database-schedule ?

@robersonfaria
Copy link
Owner

I do believe it makes sense, my only concern is how to release this version, whether as a minor or major version.

I haven't been able to analyze yet if this modification can impact any application in production.

It doesn't seem to have an impact, but I haven't really stopped to analyze it, do you see any possible impact?

@thyseus
Copy link
Collaborator Author

thyseus commented Feb 19, 2022

You are right, its a kind of breaking change. We should either create a 'Changelog' chapter in the README.md or a CHANGELOG.md file describing what we changed. A 1.3 instead of 2.0 is appropriate in my opinion.

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.

2 participants