Visitar URL original
Terminus site backup get --element=db #215 by jcandan · Pull Request #217 · pantheon-systems/terminus · GitHub
Skip to content

Terminus site backup get --element=db #215 - #217

Merged
SaraMcCutcheon merged 9 commits into
pantheon-systems:masterfrom
jcandan:terminus-site_backup_get_element_db-215
Jun 18, 2015
Merged

SaraMcCutcheon merged 9 commits into
pantheon-systems:masterfrom
jcandan:terminus-site_backup_get_element_db-215

Conversation

@jcandan

@jcandan jcandan commented Apr 13, 2015

Copy link
Copy Markdown
Contributor

This addresses issue #215

@mikevanwinkle

Copy link
Copy Markdown
Contributor

@jcandan quick question, why not just normalize the value at the site() function of php/commands/site.php. Just curious. This looks good to me.

Comment thread php/commands/site.php Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, should we allow 'database' as a valid input here so as not to break existing scripts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

valid, adding.

@jcandan

jcandan commented Apr 14, 2015

Copy link
Copy Markdown
Contributor Author

not sure what you mean by "normalize the value at the site() function of php/commands/site.php". could you elaborate?

@jcandan

jcandan commented Apr 14, 2015

Copy link
Copy Markdown
Contributor Author

@mikevanwinkle made suggested improvement. let me know your thots. thx!

@jcandan

jcandan commented Apr 14, 2015

Copy link
Copy Markdown
Contributor Author

This also addresses #216.

@mikevanwinkle

Copy link
Copy Markdown
Contributor

@jcandan I just meant that if you modify site.php to transform "database" into "db" then all the changes to the Environment file seem to be unnecessary. I feel like one place of the other should be responsible for the transformation and not both and I was just curious why you picked Environment.php instead of site.php.

@jcandan

jcandan commented Apr 14, 2015

Copy link
Copy Markdown
Contributor Author

@mikevanwinkle I guess I was thinking it'd be best to correct the bug, since db is what the help documentation shows, and is used correctly in other places. I only added the transform for backwards compatibility (good idea btw). however, one plus with this setup is that at some point in the future you can deprecate and remove database as a valid option.

@jcandan

jcandan commented Apr 21, 2015

Copy link
Copy Markdown
Contributor Author

@mikevanwinkle, so, what do you think?
A) should i remove the work done in Environment, and we just go with a handshake in site.php to translate db to database? or
B) do you like the idea of Environment correctly taking db as per documentation, and site.php translates database to db for backward compatibility (as is currently done in my patch)?

@jcandan

jcandan commented Apr 30, 2015

Copy link
Copy Markdown
Contributor Author

@mikevanwinkle so, no go, eh?

@SaraMcCutcheon

Copy link
Copy Markdown
Contributor

This looks good,, @jcandan. Thanks! I'll merge it into master.

SaraMcCutcheon pushed a commit that referenced this pull request Jun 18, 2015
…_db-215

Terminus site backup get --element=db #215
@SaraMcCutcheon
SaraMcCutcheon merged commit 69c70d0 into pantheon-systems:master Jun 18, 2015
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.

3 participants