Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Add a simple CSV env var processor#25627
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
javiereguiluz commentedDec 29, 2017
I consider that the |
nicolas-grekas left a comment
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.
Nice :)
nicolas-grekas commentedDec 29, 2017
(fixture needs an update to make tests green) |
dunglas commentedDec 29, 2017
I don't get how to make the test green. The base64 test is green and it's almost the same. Do you have a hint? |
| { | ||
| private $parameters; | ||
| private $targetDirs = array(); | ||
| private $privates = array(); |
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.
--- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_csv_env.php+++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_csv_env.php@@ -18,7 +18,11 @@ class Symfony_DI_PhpDumper_Test_CsvParameters extends Container { private $parameters; private $targetDirs = array();- private $privates = array();++ /**+ * @internal but protected for BC on cache:clear+ */+ protected $privates = array(); public function __construct() {
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.
But then it will fail for master right?
nicolas-grekasDec 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I'm not sure what you mean: this is for master already.
nicolas-grekas commentedJan 3, 2018
@dunglas fabbot failure here |
nicolas-grekas commentedJan 3, 2018
OH, in fact this CS fix has already been done on master AFAIK, so please just rebase to relaunch tests. |
ostrolucky commentedJan 6, 2018
Can we rename to |
nicolas-grekas commentedJan 7, 2018
@ostrolucky "csv_row" looks like noisy boilerplate to me. Native's |
fabpot commentedJan 19, 2018
Thank you@dunglas. |
This PR was squashed before being merged into the 4.1-dev branch (closes#25627).Discussion----------[DI] Add a simple CSV env var processor| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aAdd a new environment variable processor to parse the very popular comma separated array format like `foo,bar`. As it uses the `str_getcsv`, it also support escaping and enclosure.I'm not sure about the name, maybe `array` or `simple_array` would be better.Commits-------d730209 [DI] Add a simple CSV env var processor
Uh oh!
There was an error while loading.Please reload this page.
Add a new environment variable processor to parse the very popular comma separated array format like
foo,bar. As it uses thestr_getcsv, it also supports escaping and enclosure.I'm not sure about the name, maybe
arrayorsimple_arraywould be better.