-
Notifications
You must be signed in to change notification settings - Fork 46
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
[WIP] Add a self-update
command to update local phpmnd.phar from Github releases
#44
base: master
Are you sure you want to change the base?
Conversation
Add comments to explain consts Exclude codesniffer from PHARs Stash
src/Console/Command/SelfUpdate.php
Outdated
->addOption( | ||
'stable', | ||
's', | ||
InputOption::VALUE_NONE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be deleted. If support is needed for non-stable releases (alphas, betas, RCs) alternative stability flags can be passed into phar-updater.
I need to take a better look on this :) |
src/Console/Command/SelfUpdate.php
Outdated
const PACKAGE_NAME = 'povils/phpmnd'; | ||
|
||
/** | ||
* This is the remote file name, not local name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove comment and name constant REMOTE_FILENAME :)
src/Console/Command/SelfUpdate.php
Outdated
/** | ||
* Packagist package name | ||
*/ | ||
const PACKAGE_NAME = 'povils/phpmnd'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this belongs to Application i think so
src/Console/Command/SelfUpdate.php
Outdated
{ | ||
|
||
/** | ||
* Packagist package name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PACKAGIST_PACKAGE_NAME and you dont need comment
src/Console/Command/SelfUpdate.php
Outdated
} | ||
|
||
/** | ||
* Configure phar-updater with local phar details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment doesnt match with method name. In other words get rid of comment :)
$stability, | ||
$updater->getNewVersion() | ||
)); | ||
} elseif (false == $updater->getNewVersion()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getNewVersion returns bool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it does. There's an implementation shortfall in the phar-updater library, where this avoids calling Packagist twice. Something to be fixed there in a future version.
use Humbug\SelfUpdate\Updater; | ||
use Humbug\SelfUpdate\Strategy\GithubStrategy; | ||
|
||
class SelfUpdate extends BaseCommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make protected methods,properties to private :)
src/Console/Command/SelfUpdate.php
Outdated
try { | ||
$result = $updater->rollback(); | ||
if ($result) { | ||
$this->output->writeln('<fg=green>PHPMND has been rolled back to prior version.</fg=green>'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is different from <fg=green> ? I would prefer use , , and so on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your preference didn't show up in comment (probably github markdown parser rendering as HTML tag).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn :D <info>, <danger>, <warning>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D Updated. Check if those are in line with what you would prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, seem to be getting ignored for some reason in actual output (at least here). May need to check that tomorrow.
@@ -36,4 +36,28 @@ if (false === $loaded) { | |||
ini_set('xdebug.max_nesting_level', 10000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one file is the one i need to take a better look :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it doesn't sit right here, but can't think of a better way via Symfony/Console. We're basically checking if no command is sent, and prefixing run
. We have a run
command since adding multiple commands (even two for self-update) seems to require abandoning a default command. Also, the using setDefaultCommand
method for Application doesn't accept arguments so that's not a solution.
Not sure if these are specific design decisions (or just limitations) by the Symfony team, or I'm just missing something obvious that would make that prepareArgv
function go away.
Thanks for the review @povils - made some changes. Missed your comment on the text colouring if you have specific colour codes to fit in with your preferred theme. |
@povils Pretty much done, but I may add a few changes towards the weekend to take advantage of a new phar-updater release expected shortly, and someone may know if the fugly argv function has a more...elegant solution. |
self-update
command (not loaded unless running a phar). Alsoself-update --check
(check if new version) andself-update --rollback
(revert to previous version).phpmnd
command torun
(bash script edited to inject sophpmnd
works exactly as-is).--help
and--version
/-V
as expected.padraic/phar-updater
added.Noted as WIP for feedback. Having a
run
command - there's probably a better way to have a default command (where$_SERVER['argv']
isn't manipulated outside of symfony/console). This also omits theself-update
command from--help
.Works, but may need some tidying for those two points.