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

Added to the basic app, fixed config, added to core commands. #1759

Merged
merged 3 commits into from
Jan 3, 2014
Merged

Added to the basic app, fixed config, added to core commands. #1759

merged 3 commits into from
Jan 3, 2014

Conversation

Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Jan 3, 2014

I added to the basic application console.php config to avoid unnecessary exception with fixtures path is not configured right and added db config (db is needed for apply/clear actions), i also added folder templates where by default will be hosted templates for yii2-faker, so user should not worry how to configure it and everything will be by default for him.
Added fixture to the core commands. Improved some messages and checks.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Jan 3, 2014

also @qiangxue do we need to have some $this->confirm() for user when he trying to apply/clear actions of fixture controller? i think it would be another useful extra-check for user, and should be implemented. What do you think?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Jan 3, 2014

Related with #1756

@@ -19,6 +22,17 @@
],
],
],
'db' => [
'class' => 'yii\db\Connection',
'dsn' => 'mysql:host=localhost;dbname=yii2basic',
Copy link
Member

Choose a reason for hiding this comment

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

Please use yii2_basic to be consistent with DB setting in other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Just one comment...
Maybe it would be cleaner to separate db config in a separated file, just as $params.
That would help sharing (and versioning) the main config file wile each developer has it's own database configuration (ignored by CVS) without problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qiangxue should it be?

Copy link
Member

Choose a reason for hiding this comment

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

this is not part of this PR in general it is a good idea. Not sure if we should do it in basic app. Discussion should go in a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, could you maybe create it?

@qiangxue
Copy link
Member

qiangxue commented Jan 3, 2014

I think we need confirm() for any command that may cause change of the system.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Jan 3, 2014

I think we need confirm() for any command that may cause change of the system.

ok.

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Jan 3, 2014

Added confirmation. Ready for review. No sure if we for example need to add outputList method to basic console controller, anyway could be helpful, but ofcourse i am against any mess that is out of domain scope.

{
if ($this->getFixtureManager() == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

== => ===

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

@Ragazzo
Copy link
Contributor Author

Ragazzo commented Jan 3, 2014

Done, can be reviewed.

cebe added a commit that referenced this pull request Jan 3, 2014
Added to the basic app, fixed config, added to core commands.
@cebe cebe merged commit 08ee8b1 into yiisoft:master Jan 3, 2014
@cebe
Copy link
Member

cebe commented Jan 3, 2014

Thanks!

@Ragazzo Ragazzo deleted the fixtures_controller_improvements branch January 3, 2014 22:24
@Ragazzo
Copy link
Contributor Author

Ragazzo commented Jan 3, 2014

@cebe also change alias to dirname in tests/_bootstrap.php. Thanks.

@cebe
Copy link
Member

cebe commented Jan 3, 2014

tests/_bootstrap.php is already fine.

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.

5 participants