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

[Console] Fix Console QuestionHelper $app to $this #3952

Closed
wants to merge 1 commit into from

Conversation

eko
Copy link
Contributor

@eko eko commented Jun 13, 2014

On QuestionHelper page, commands examples for calling getHelperSet use the following (where $app is undefined):

$app->getHelperSet()->get('question');

I've changed it to correct:

$this->getHelperSet()->get('question');

@cordoval
Copy link
Contributor

true although you could have gotten the $app from $app = $this->getApplication(), also notice that $this->getApplication() is tagged @api so it is here to stay. But yeah this is a 👍

@wouterj
Copy link
Member

wouterj commented Jun 14, 2014

👍 we never told what the $app variable contains, so it is confusing. Thanks for fixing it!

However, maybe changing it to $this->getHelper('question') is even better? (but that can be done in another PR too)

Fix getHelperSet() to getHelper()

Typo fix

Move getHelperSet() to getHelper() in other files
@eko
Copy link
Contributor Author

eko commented Jun 14, 2014

Yes, it was confusing.

I've updated the PR to use $this->getHelper('...') instead of $this->getHelperSet()->get('...')

@cordoval
Copy link
Contributor

👍

@weaverryan
Copy link
Member

Hi guys!

Yes, this is much better. But, I need your help! Most of these changes apply to all versions (back to 2.3), but some (QuestionHelper) only apply to Symfony 2.5. Here's what we need to do:

  1. Rebase this PR against the 2.3 branch (using rebase --onto...) or create a new PR against the 2.3 branch with only the 2.3 changes
  2. Create a second PR with the 2.5-specific features.

@eko Can you help out with this?

Thanks!

@eko
Copy link
Contributor Author

eko commented Jun 21, 2014

Hi @weaverryan,

Alright, I will create 2 new PRs against 2.3 and 2.5 branches in the week-end.

Thank you

@weaverryan
Copy link
Member

Thanks @eko!

@eko
Copy link
Contributor Author

eko commented Jul 5, 2014

Hi @weaverryan,

I've sent the 2 PRs against 2.3 and 2.5 branches:

So I think this PR can be closed?

Thank you

@wouterj wouterj closed this Jul 11, 2014
weaverryan added a commit that referenced this pull request Jul 11, 2014
…getHelper() (eko)

This PR was merged into the 2.3 branch.

Discussion
----------

[Console] Fix Console component getHelperSet()->get() to getHelper()

| Q                  | A          |
| ---------------- |:---------:|
| Doc fix?        | Yes |
| New docs?   | No   |
| Applies to     | 2.3   |
| Fixed tickets | No   |

On Console component, I've fixed the following calls:

```php
$this->getHelperSet()->get('dialog');
```

to:

```php
$this->getHelper('dialog');
```

This is related to original PR #3952

Commits
-------

9207f68 [Console] Fix Console component getHelperSet()->get() to getHelper()
weaverryan added a commit that referenced this pull request Jul 11, 2014
…getHelper() method (eko)

This PR was merged into the 2.5 branch.

Discussion
----------

[Console] Fix Console component $app to $this and use of getHelper() method

| Q                  | A          |
| ---------------- |:---------:|
| Doc fix?        | Yes |
| New docs?   | No   |
| Applies to     | 2.5   |
| Fixed tickets | No   |

On Console component, I've fixed the following:

### Use of getHelper() method

```php
$this->getHelperSet()->get('dialog');
```

to:

```php
$this->getHelper('dialog');
```

### Also fixed unusued $app variable to $this

This is related to original PR #3952

Commits
-------

1f4dc76 [Console] Fix Console some $app to $this and getHelperSet()->get() to getHelper()
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.

4 participants