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 propel init command #693

Merged
merged 1 commit into from Sep 16, 2014

Conversation

Projects
None yet
7 participants
@mpscholten
Member

mpscholten commented Aug 4, 2014

bildschirmfoto 2014-08-04 um 19 02 31

Based on the discussion in #679.
This is currently work in progress but I want to get some early feedback.

Still to do:

  • Check if we can connect to the db with the credentials provided by the user
  • Allow running reverse-task on existing databases via init command (suggested by @BunnyHolder)
  • Add DSN generation for all rdbms's (currently only mysql)
  • Add prod/dev context to generated propel.yml (we should probably generate a propel.yml.dist instead of a propel.yml, or we could use env-vars for passing the credentials, not sure on this)
  • Add missing templates for config formats other than yml and xml

So what do you think of this? Let me know if you have some suggestions :)

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm Aug 4, 2014

Member

Really like what I see.

Since the refactoring for the config system landed, people can define the config in different formats.
We should ask for the format.

Should we also ask for encoding of the app/db?

Member

staabm commented Aug 4, 2014

Really like what I see.

Since the refactoring for the config system landed, people can define the config in different formats.
We should ask for the format.

Should we also ask for encoding of the app/db?

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Aug 4, 2014

Member

I really really really like it! Please continue :)

Member

marcj commented Aug 4, 2014

I really really really like it! Please continue :)

@nymo

This comment has been minimized.

Show comment
Hide comment
@nymo

nymo Aug 4, 2014

Contributor

👍 for asking for the encoding

Contributor

nymo commented Aug 4, 2014

👍 for asking for the encoding

@BunnyHolder

This comment has been minimized.

Show comment
Hide comment
@BunnyHolder

BunnyHolder Aug 5, 2014

Default value for namespace maybe Base?

Maybe you should ask, will it have many connections, or envs? Or how much envs it will have.

Currently I love it :)

P.S. There should be sql_mode check for mysql: #685 (comment)

And let's not forgot asking for demo scheme and demo data

BunnyHolder commented Aug 5, 2014

Default value for namespace maybe Base?

Maybe you should ask, will it have many connections, or envs? Or how much envs it will have.

Currently I love it :)

P.S. There should be sql_mode check for mysql: #685 (comment)

And let's not forgot asking for demo scheme and demo data

@robin850

This comment has been minimized.

Show comment
Hide comment
@robin850

robin850 Aug 5, 2014

Member

This is really one of the most important missing feature in Propel ! Awesome ! 👍

Member

robin850 commented Aug 5, 2014

This is really one of the most important missing feature in Propel ! Awesome ! 👍

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Aug 5, 2014

Member

Thanks for the positive feedback :)

Since the refactoring for the config system landed, people can define the config in different formats.
We should ask for the format.

👍

Should we also ask for encoding of the app/db?

I suggest to use utf8 by default, convention over configuration. If the user really need to use a custom encoding he can still change the propel.yml

Default value for namespace maybe Base?

I think we could try to detect the namespace like we are detecting the phpDir :)

Maybe you should ask, will it have many connections, or envs? Or how much envs it will have.

We should probably add prod dev and test by default to the propel.yml. If the user needs more he can just add one by hand.

Member

mpscholten commented Aug 5, 2014

Thanks for the positive feedback :)

Since the refactoring for the config system landed, people can define the config in different formats.
We should ask for the format.

👍

Should we also ask for encoding of the app/db?

I suggest to use utf8 by default, convention over configuration. If the user really need to use a custom encoding he can still change the propel.yml

Default value for namespace maybe Base?

I think we could try to detect the namespace like we are detecting the phpDir :)

Maybe you should ask, will it have many connections, or envs? Or how much envs it will have.

We should probably add prod dev and test by default to the propel.yml. If the user needs more he can just add one by hand.

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm Aug 5, 2014

Member

I suggest to use utf8 by default, convention over configuration. If the user really need to use a custom encoding he can still change the propel.yml

you could ask for it and use utf8 as a default (like you already do for the db user which defaults to root)

Member

staabm commented Aug 5, 2014

I suggest to use utf8 by default, convention over configuration. If the user really need to use a custom encoding he can still change the propel.yml

you could ask for it and use utf8 as a default (like you already do for the db user which defaults to root)

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Aug 9, 2014

Member

Currently we need to wait for #679 to be resolved. Otherwise reverse engineering won't work.

Here's the current state of the command:
bildschirmfoto 2014-08-10 um 00 01 24

In some cases I didn't implemented auto dsn generation because there were to many options (e.g. http://php.net/manual/de/ref.pdo-sqlsrv.connection.php (to many options) or http://php.net/manual/de/ref.pdo-dblib.connection.php (to complicated, charset depends on some php extensions being installed, see comments on the linked php.net page)). If you pick one of these drivers you will be prompted to enter the dsn manually instead of entering the database, host, etc.

Member

mpscholten commented Aug 9, 2014

Currently we need to wait for #679 to be resolved. Otherwise reverse engineering won't work.

Here's the current state of the command:
bildschirmfoto 2014-08-10 um 00 01 24

In some cases I didn't implemented auto dsn generation because there were to many options (e.g. http://php.net/manual/de/ref.pdo-sqlsrv.connection.php (to many options) or http://php.net/manual/de/ref.pdo-dblib.connection.php (to complicated, charset depends on some php extensions being installed, see comments on the linked php.net page)). If you pick one of these drivers you will be prompted to enter the dsn manually instead of entering the database, host, etc.

@nymo

This comment has been minimized.

Show comment
Hide comment
@nymo

nymo Aug 9, 2014

Contributor

I really like the current state, good work.

If you pick one of these drivers you will be prompted to enter the dsn manually instead of entering the database, host, etc.

What about proposing at least a link to the manual to show the user where he can lookup the correct dsn format?

Contributor

nymo commented Aug 9, 2014

I really like the current state, good work.

If you pick one of these drivers you will be prompted to enter the dsn manually instead of entering the database, host, etc.

What about proposing at least a link to the manual to show the user where he can lookup the correct dsn format?

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Aug 9, 2014

Member

What about proposing at least a link to the manual to show the user where he can lookup the correct dsn format?

That's already implemented :) The prompt links to http://php.net/manual/de/pdo.drivers.php, but we should propably redirect the user to the exact dsn url, like this: http://php.net/manual/de/ref.pdo-dblib.connection.php#refsect1-ref.pdo-dblib.connection-description

Member

mpscholten commented Aug 9, 2014

What about proposing at least a link to the manual to show the user where he can lookup the correct dsn format?

That's already implemented :) The prompt links to http://php.net/manual/de/pdo.drivers.php, but we should propably redirect the user to the exact dsn url, like this: http://php.net/manual/de/ref.pdo-dblib.connection.php#refsect1-ref.pdo-dblib.connection-description

@nymo

This comment has been minimized.

Show comment
Hide comment
@nymo

nymo Aug 9, 2014

Contributor

Very predictive :) a direct link to the explicit manual would be better to avoid confusion for the user if it's not to complicated to implement.

Contributor

nymo commented Aug 9, 2014

Very predictive :) a direct link to the explicit manual would be better to avoid confusion for the user if it's not to complicated to implement.

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Aug 10, 2014

Member

Just added more specific help links 👍

Member

mpscholten commented Aug 10, 2014

Just added more specific help links 👍

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Aug 18, 2014

Member

Just added the missing config file formats (ini, xml, php and json). Currently we generate one propel.ext and one propel.ext.dist file for handling different environments. We could also use env-parameters for that, so what is the best practice on this? :)

Otherwise I think I'm ready for review.

Member

mpscholten commented Aug 18, 2014

Just added the missing config file formats (ini, xml, php and json). Currently we generate one propel.ext and one propel.ext.dist file for handling different environments. We could also use env-parameters for that, so what is the best practice on this? :)

Otherwise I think I'm ready for review.

@mpscholten mpscholten changed the title from [WIP] Added propel init command to Added propel init command Aug 18, 2014

@cristianoc72

This comment has been minimized.

Show comment
Hide comment
@cristianoc72

cristianoc72 Aug 19, 2014

Member

Well, propel.ext.dist is needed only if we don't want to commit some config
options in a shared repository. So imho we could generate only propel.ext.

Member

cristianoc72 commented Aug 19, 2014

Well, propel.ext.dist is needed only if we don't want to commit some config
options in a shared repository. So imho we could generate only propel.ext.

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Aug 19, 2014

Member

Isn't it best practice to not commit database passwords into the repository?

Member

mpscholten commented Aug 19, 2014

Isn't it best practice to not commit database passwords into the repository?

@cristianoc72

This comment has been minimized.

Show comment
Hide comment
@cristianoc72

cristianoc72 Aug 19, 2014

Member

Of course yes! I meant: a programmer working in team and using a cvs is supposed to be skilled enough to create its own propel.ext.dist.
Anyway, IMHO your choice is ok. This is the best practice and your awesome work is done!

Member

cristianoc72 commented Aug 19, 2014

Of course yes! I meant: a programmer working in team and using a cvs is supposed to be skilled enough to create its own propel.ext.dist.
Anyway, IMHO your choice is ok. This is the best practice and your awesome work is done!

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Aug 19, 2014

Member

I meant: a programmer working in team and using a cvs is supposed to be skilled enough to create its own propel.ext.dist.

You're right but I thought about this command as a starting point for any new propel projects :-) Therefore we should follow best practices where possible to make starting a new propel project as easy as possible.

Anyway, IMHO your choice is ok. This is the best practice and your awesome work is done!

Thanks for your feedback. If you have any other thoughts, let me know!

Member

mpscholten commented Aug 19, 2014

I meant: a programmer working in team and using a cvs is supposed to be skilled enough to create its own propel.ext.dist.

You're right but I thought about this command as a starting point for any new propel projects :-) Therefore we should follow best practices where possible to make starting a new propel project as easy as possible.

Anyway, IMHO your choice is ok. This is the best practice and your awesome work is done!

Thanks for your feedback. If you have any other thoughts, let me know!

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Sep 14, 2014

Member

@marcj is there anything missing before we can merge this?

Member

mpscholten commented Sep 14, 2014

@marcj is there anything missing before we can merge this?

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Sep 16, 2014

Member

@mpscholten, yeah, test suite needs to be green :-)

Member

marcj commented Sep 16, 2014

@mpscholten, yeah, test suite needs to be green :-)

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Sep 16, 2014

Member

They indeed should be green :) but I've never touched the failing test. Could you try to restart travis to check if it's not just a "one-time" failure?

Member

mpscholten commented Sep 16, 2014

They indeed should be green :) but I've never touched the failing test. Could you try to restart travis to check if it's not just a "one-time" failure?

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Sep 16, 2014

Member

Yepp, restarted it already 15min ago.

Member

marcj commented Sep 16, 2014

Yepp, restarted it already 15min ago.

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Sep 16, 2014

Member

Ok thanks. Just rebased again. I'll let you know when the failing test is fixed :)

Member

mpscholten commented Sep 16, 2014

Ok thanks. Just rebased again. I'll let you know when the failing test is fixed :)

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Sep 16, 2014

Member

@marcj tests are green.

The cause of this bug was the Fixture loader trying to load the DialogHelper as a command. Later a few tests were trying to use these fixture, but they weren't loaded, so these tests failed.

This was fixed by applying a depth(0) to the finder used by the Fixture loader.

Member

mpscholten commented Sep 16, 2014

@marcj tests are green.

The cause of this bug was the Fixture loader trying to load the DialogHelper as a command. Later a few tests were trying to use these fixture, but they weren't loaded, so these tests failed.

This was fixed by applying a depth(0) to the finder used by the Fixture loader.

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Sep 16, 2014

Member

Really cool. If you rebase it then I'm going to merge this cool stuff 8-)

Member

marcj commented Sep 16, 2014

Really cool. If you rebase it then I'm going to merge this cool stuff 8-)

Added propel init command
Added more help text

Fixed detectDefaultPhpDir not setting a max depth when searching for files

Added dsn generation for pgsql and sqlite

Added charset

Fixed whitespaces in generated propel.yml

Added xml configuration

Added writeable check

Improved order of texts and added reverse

Replaced file_put_contents with symfony2 filesystem methods, added missing platforms, fixed formatting of Dialog::select style

Test connection before continuing with the set up

Setup db connection first and define config file format at the end of the process, also fixed sql errors to be to unspecific while testing the connection

Fixed formatting of confirmation questions

Added config format to summary

Improved help links for manual entered dsn strings

Added config.ext.dist and improved reverse engineering

Added missing config file templates

Fixed test case fixtures which caused other tests to fail with obscure error messages
@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Sep 16, 2014

Member

Here we go :)

Member

mpscholten commented Sep 16, 2014

Here we go :)

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm Sep 16, 2014

Member

❤️

Member

staabm commented Sep 16, 2014

❤️

@mpscholten

This comment has been minimized.

Show comment
Hide comment
@mpscholten

mpscholten Sep 16, 2014

Member

❤️

By the way: After the merge we probably should add a few notes to the docs.

Member

mpscholten commented Sep 16, 2014

❤️

By the way: After the merge we probably should add a few notes to the docs.

marcj added a commit that referenced this pull request Sep 16, 2014

Merge pull request #693 from mpscholten/propel-init
Added propel init command

@marcj marcj merged commit 7aaf360 into propelorm:master Sep 16, 2014

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Sep 16, 2014

Member

Bäm, 🚢ped very good work @mpscholten <3

Member

marcj commented Sep 16, 2014

Bäm, 🚢ped very good work @mpscholten <3

@marcj marcj referenced this pull request Sep 16, 2014

Closed

Docu for command init #323

@mpscholten mpscholten deleted the mpscholten:propel-init branch Sep 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment