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

Fix for #8759 (ability to specify custom fixture suite during install) #9967

Merged
merged 7 commits into from Mar 21, 2019

Conversation

Projects
None yet
4 participants
@igormukhingmailcom
Copy link
Contributor

commented Nov 26, 2018

Q A
Branch? master
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes #8759
License MIT

@igormukhingmailcom igormukhingmailcom requested a review from Sylius/core-team as a code owner Nov 26, 2018

@igormukhingmailcom

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

Guys, I also suggest to not ask confirmation question if suite was specified (at https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/CoreBundle/Command/InstallSampleDataCommand.php#L55) like this:

if (!$questionHelper->ask(
    $input, 
    $output, 
    new ConfirmationQuestion(
        'Continue? (y/N) ', 
        null !== $suite // <------------- Question will be answered YES by default in no interaction mode if suite was provided
    )
)) {

This not breaks BC as far as new option/$suite is null by default (so null !== $suite is false by default as it was before), but allows us to run install commands with --fixture-suite=custom --no-interaction options and load fixtures automatically (now, if you run install with --no-interaction option - sample data loading step will be skipped).

@igormukhingmailcom igormukhingmailcom changed the base branch from master to 1.2 Nov 27, 2018

@lchrusciel
Copy link
Member

left a comment

Thanks a lot for contribution Igor!

I can totally agree with your suggestion, may you add it to your PR? Also, as this is a new feature I will change the branch to master.

@lchrusciel lchrusciel changed the base branch from 1.2 to master Nov 28, 2018

@igormukhingmailcom

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

Hi, @lchrusciel
I've fixed build fail

@Zales0123 Zales0123 added the Feature label Nov 30, 2018

@stale

This comment has been minimized.

Copy link

commented Mar 12, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale label Mar 12, 2019

@igormukhingmailcom

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Hi, @Zales0123 .
Any chance to merge this?

@stale stale bot removed the Stale label Mar 12, 2019

@mamazu

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

That would indeed be a nice feature to have.

@igormukhingmailcom

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@Zales0123 @lchrusciel Could any of you review please?

@lchrusciel

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

I'm ok with the changes, thanks a lot for the contribution 👍

@Zales0123 Zales0123 merged commit e0cb477 into Sylius:master Mar 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Zales0123

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Thank you, Igor! 🏅

lchrusciel pushed a commit to Sylius/SyliusCoreBundle that referenced this pull request Mar 21, 2019

lchrusciel pushed a commit to Sylius/SyliusCoreBundle that referenced this pull request Mar 21, 2019

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