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

properly use the cli helper #74

Merged
merged 3 commits into from
Dec 28, 2013
Merged

properly use the cli helper #74

merged 3 commits into from
Dec 28, 2013

Conversation

dbu
Copy link
Member

@dbu dbu commented Jun 26, 2013

we should not create an instance of the phpcr_cli helper manually. i wonder why we even have both PhpcrHelper and PhpcrCliHelper - can we just merge the additional functionality into PhpcrHelper instead? /cc @dantleech

@dbu
Copy link
Member Author

dbu commented Jun 26, 2013

once this is sorted out, we might also need to update the DoctrinePHPCRBundle and both jackalope cli-config.php.dist

@dbu
Copy link
Member Author

dbu commented Jun 26, 2013

ups so the tests are not set up correctly. but if we merge the two helpers i don't need to look into that.

@dantleech
Copy link
Member

Yeah.. so we instantiate the Helper manually because it needs to be present when the DoctrinePhpcrBundle instantiates the Commands.

The PhpcrHelper is replaced in a nasty way by the DoctrinePhpcrBundle:

https://github.com/doctrine/DoctrinePHPCRBundle/blob/master/Command/DoctrineCommandHelper.php

We cannot move the logic into the PhpcrHelper because of this.

There is also no way to automatically insert "helpers" into commands, for example, with DI - so thats why I instantiate it manually, and also I think why the "dump" helper was instantiated manually too.

@dbu dbu mentioned this pull request Jun 28, 2013
@dbu
Copy link
Member Author

dbu commented Jun 28, 2013

i will look into this in the context of the sandbox then and try to clean all this up. the DoctrineCommandHelper indeed looks rather scary.

@lsmith77
Copy link
Member

ping

@dbu
Copy link
Member Author

dbu commented Jul 11, 2013

i did not get around to do this. and i won't have time today i fear, so lets tag without the fix. @dantleech did a quickfix last week to make the commands instantiate some helpers on the fly so at least it usually works now.

for when i get to do it: what is the sane way for symfony commands to get their helpers? is there some listener or something we can use, or is the funny static call to a helper the only way to do this?

@dantleech
Copy link
Member

@wouterj suggested that I make a PR to Symfony to enable helpers to be registered via. DI. At the moment the only way to register helpers AFAIK is to add them manually the Application.

@lsmith77
Copy link
Member

ping

Conflicts:
	src/PHPCR/Util/Console/Command/BaseCommand.php
@dbu
Copy link
Member Author

dbu commented Dec 26, 2013

resurrected this, and cleaned up everything. the adjustments in phpcr-odm and phpcr-bundle are minimal, but i think there is some BC break in the setup, so i up the version to 1.1 here.

@lsmith77
Copy link
Member

@dantleech no objections from my side .. lets merge?

@@ -116,7 +117,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
$parentPath
));

return;
return 2;
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity - why 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

a non 0 return code of a linux binary means an error state. i just counted up the return statements so a calling programm could distinguish the states if it cares.

@dantleech
Copy link
Member

Looks good to me.

lsmith77 added a commit that referenced this pull request Dec 28, 2013
@lsmith77 lsmith77 merged commit 843d22f into master Dec 28, 2013
@lsmith77 lsmith77 deleted the command-helpers branch December 28, 2013 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants