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

[WIP] Add a self-update command to update local phpmnd.phar from Github releases #44

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

padraic
Copy link
Contributor

@padraic padraic commented May 6, 2017

  • Adds a self-update command (not loaded unless running a phar). Also self-update --check (check if new version) and self-update --rollback (revert to previous version).
  • Renames default phpmnd command to run (bash script edited to inject so phpmnd works exactly as-is).
  • Some remapping to allow --help and --version/-V as expected.
  • Updates are linked to stable Github releases (no version constraints included).
  • Dependency to padraic/phar-updater added.
  • PHP_CodeSniffer add to exclude list when creating a PHAR.

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 the self-update command from --help.

Works, but may need some tidying for those two points.

->addOption(
'stable',
's',
InputOption::VALUE_NONE,
Copy link
Contributor Author

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.

@povils
Copy link
Owner

povils commented May 7, 2017

I need to take a better look on this :)

const PACKAGE_NAME = 'povils/phpmnd';

/**
* This is the remote file name, not local name.
Copy link
Owner

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 :)

/**
* Packagist package name
*/
const PACKAGE_NAME = 'povils/phpmnd';
Copy link
Owner

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

{

/**
* Packagist package name
Copy link
Owner

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

}

/**
* Configure phar-updater with local phar details.
Copy link
Owner

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()) {
Copy link
Owner

Choose a reason for hiding this comment

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

getNewVersion returns bool?

Copy link
Contributor Author

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
Copy link
Owner

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 :)

try {
$result = $updater->rollback();
if ($result) {
$this->output->writeln('<fg=green>PHPMND has been rolled back to prior version.</fg=green>');
Copy link
Owner

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

Copy link
Contributor Author

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).

Copy link
Owner

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>

Copy link
Contributor Author

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.

Copy link
Contributor Author

@padraic padraic May 7, 2017

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);
Copy link
Owner

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 :)

Copy link
Contributor Author

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.

@padraic
Copy link
Contributor Author

padraic commented May 7, 2017

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.

@padraic
Copy link
Contributor Author

padraic commented May 16, 2017

@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.

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.

None yet

2 participants