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

Add description of autocompleter callback #11349

Merged
merged 1 commit into from Apr 9, 2019

Conversation

Projects
None yet
6 participants
@MikkelPaulson
Copy link
Contributor

commented Apr 8, 2019

Documented feature added by symfony/symfony#30997.

@@ -179,6 +179,46 @@ will be autocompleted as the user types::
$bundleName = $helper->ask($input, $output, $question);
}

In more complex use cases, it may be necessary to generate suggestions on the

This comment has been minimized.

Copy link
@Toflar

Toflar Apr 8, 2019

Contributor

I‘m not sure this is the best example. What you‘re doing is passing on user input to scandir() which opens the door for a dot dot slash attack. Sure, it‘s your own system but who knows who‘s going to read the docs and it what context the reader is using the example :) wdyt?

This comment has been minimized.

Copy link
@MikkelPaulson

MikkelPaulson Apr 8, 2019

Author Contributor

Well, I added the feature because I wanted to autocomplete subsequent words in a command chain, but the most obvious use case seems to be path autocompletion (particularly since that's behaviour we're accustomed to in bash as well).

I'm open to alternatives, but that's the best I could come up with in terms of something that was a) useful, and b) straightforward enough to be readable as an example.

Maybe a disclaimer would be an appropriate compromise?

symfony-splitter pushed a commit to symfony/console that referenced this pull request Apr 8, 2019

feature #30997 [Console] Add callback support to Console\Question aut…
…ocompleter (Mikkel Paulson)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Console] Add callback support to Console\Question autocompleter

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | minor edge case, see below
| Deprecations? | no
| Tests pass?   | yes (with expanded coverage)
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | symfony/symfony-docs#11349

Autocompletion is a useful feature, but it's not always possible to anticipate every input the user could provide in advance. For instance, if we're allowing the user to input a path to a file, it's not practical to populate an array with every file and directory in the filesystem, but we can easily build a callback function that populates its suggestions based on the path already inputted.

This change replaces the autocomplete logic that accepts an array of suggestions with an architecture that uses a callback function to populate suggestions in real time as the user provides input.

The first commit adds a test class covering all methods of the `Question` object, while the second commit modifies the `Question` object to accept and store a callback function. The existing `[gs]etAutocompleterValues()` methods are preserved, but instead of being referenced directly from the `QuestionHelper`, they create and call their own callbacks to emulate the current behaviour.

There is one edge case that is changed, as documented in the test: when a `Traversable` object is passed to `setAutocompleterValues()`, the return value of `getAutocompleterValues()` will be the unpacked (array) form of that object rather than the object itself. The unpacking is done lazily and cached on the callback function.

Commits
-------

caad562c11 [Console] Add callback support to Console\Question autocompleter

fabpot added a commit to symfony/symfony that referenced this pull request Apr 8, 2019

feature #30997 [Console] Add callback support to Console\Question aut…
…ocompleter (Mikkel Paulson)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Console] Add callback support to Console\Question autocompleter

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | minor edge case, see below
| Deprecations? | no
| Tests pass?   | yes (with expanded coverage)
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | symfony/symfony-docs#11349

Autocompletion is a useful feature, but it's not always possible to anticipate every input the user could provide in advance. For instance, if we're allowing the user to input a path to a file, it's not practical to populate an array with every file and directory in the filesystem, but we can easily build a callback function that populates its suggestions based on the path already inputted.

This change replaces the autocomplete logic that accepts an array of suggestions with an architecture that uses a callback function to populate suggestions in real time as the user provides input.

The first commit adds a test class covering all methods of the `Question` object, while the second commit modifies the `Question` object to accept and store a callback function. The existing `[gs]etAutocompleterValues()` methods are preserved, but instead of being referenced directly from the `QuestionHelper`, they create and call their own callbacks to emulate the current behaviour.

There is one edge case that is changed, as documented in the test: when a `Traversable` object is passed to `setAutocompleterValues()`, the return value of `getAutocompleterValues()` will be the unpacked (array) form of that object rather than the object itself. The unpacking is done lazily and cached on the callback function.

Commits
-------

caad562 [Console] Add callback support to Console\Question autocompleter
@MikkelPaulson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@Toflar @nicolas-grekas I've pushed a commit that addresses your feedback. Please re-review.

$filePath = $this->output->askQuestion($question);
}

Note that this example code allows unrestricted access to the host filesystem,

This comment has been minimized.

Copy link
@OskarStark

OskarStark Apr 8, 2019

Contributor

Please use a .. note:: directive instead

This comment has been minimized.

Copy link
@MikkelPaulson

MikkelPaulson Apr 8, 2019

Author Contributor

Updated to use .. caution::, which seemed more appropriate.

@Toflar

Toflar approved these changes Apr 9, 2019

@javiereguiluz javiereguiluz added this to the 4.3 milestone Apr 9, 2019

@javiereguiluz javiereguiluz merged commit 5cfc67e into symfony:master Apr 9, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
symfony SymfonyCloud: Environment deployed
Details

javiereguiluz added a commit that referenced this pull request Apr 9, 2019

minor #11349 Add description of autocompleter callback (MikkelPaulson)
This PR was merged into the master branch.

Discussion
----------

Add description of autocompleter callback

Documented feature added by symfony/symfony#30997.

Commits
-------

5cfc67e Add description of autocompleter callback
@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

@MikkelPaulson thanks a lot for providing this fantastic new feature and for documenting it too. What a great first contribution to Symfony Docs!

While merging we made some minor tweaks to your code example to make some comments more concise. Also, we moved the caution note to inside the code example, so people can't miss it. Finally, we added the versionadded directive to warn users that this is only available in Symfony 4.3 and up. Thanks!

@MikkelPaulson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Thanks everyone for the super fast feedback and merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.