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

New cv upgrade command #2

Merged
merged 51 commits into from
Mar 1, 2018
Merged

New cv upgrade command #2

merged 51 commits into from
Mar 1, 2018

Conversation

agh1
Copy link
Contributor

@agh1 agh1 commented Sep 27, 2016

@totten
Copy link
Member

totten commented May 15, 2017

@agh1 I've added a few small fixes. (Described in commit notes.)

One blocker issue in UpgradeCommand -- the calls to subprocesses pass through data without proper escaping. I suspect they work in typical usage but would break in some scenarios, like if:

  • The $problem report includes any single-quotes.
  • The $dl['url'] includes any shell characters (eg '&' is common in URLs)
  • The user passes in a $reportName or $stability with a shell characters.
  • The sys_get_temp_dir() has spaces. (Most Linux systems just use /tmp which is fine, but Win/OSX do funny things.)

Suggestion: all variables passed through to Cv::run() should be escaped with escapeshellarg().

@seancolsen
Copy link

When this is merged, it'd be nice to add an answer to this SE question which mentions the new command.

totten and others added 19 commits February 16, 2018 15:34
(1) It's confusing that several `Upgrade*Command`s show boot-related options
when they don't actually boot.

(2) It's peculiar/inefficient that the `upgrade:report` launches so many
subprocesses (`Cv::run()`).  However, it makes sense if you consider that
the `upgrade:report` needs to run inside a *non-bootstrapped* environment.
On OSX, the default behavior of rand() only produces 31 bits of entropy.
MD5 hashing is 128-bits, so we've left a lot of gaps in the 128-bit space.
(Effectively: 93 vs 128 bits.) The PHP docs say that rand() on Windows gives
even less entropy.

This revision ensures a more consistent amount of entropy.
I haven't gone through all these carefully yet, but I'd like to be able to
merge so that UpgradeDbCommand works.
@totten
Copy link
Member

totten commented Feb 18, 2018

@agh1 I've made a few updates on this PR

  1. Rebased to match latest master (and force-pushed agh1:cv-upgrade)
  2. Significantly revamped the upgrade:db command to support more detailed output. If executed interactively with the default --out=pretty, it will display a lot more info. With -v or -vv, it'll display still more. With -q, it'll be quiet. If executed with a structured format (e.g. --out=json), it should still return the same content.
  3. Temporarily disabled the other sub-commands. I think it would be useful to get something merged, and we've set the bar rather high by putting all the commands together. I think it'd be an improvement to get this merged with at least one command. (If nothing else, it'll be easier to maintain the code for all the commands... because they'll show up during grep+refactor cycles.)

@totten
Copy link
Member

totten commented Mar 1, 2018

I did a local build with #2 and #35 and ran the full cv test suite on D7/WordPress/Backdrop, and there don't appear to be any regressions here.

Let's get this merged already!

@totten totten merged commit f814f0a into civicrm:master Mar 1, 2018
@totten
Copy link
Member

totten commented Mar 1, 2018

@seanmadsen Posted a comment on https://civicrm.stackexchange.com/a/22949/93 about the cv upgrade:db command.

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