Skip to content

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

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[DI] Add a simple CSV env var processor #25627

wants to merge 2 commits into from

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Dec 29, 2017 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Add 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 supports escaping and enclosure.

I'm not sure about the name, maybe array or simple_array would be better.

@javiereguiluz
Copy link
Member

I consider that the csv name describes this processor perfectly 👍 array or simple_array would be confusing in this case.

nicolas-grekas
nicolas-grekas approved these changes Dec 29, 2017
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Nice :)

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Dec 29, 2017
@nicolas-grekas
Copy link
Member

(fixture needs an update to make tests green)

@dunglas
Copy link
Member Author

dunglas commented Dec 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?

nicolas-grekas
nicolas-grekas reviewed Dec 29, 2017
View reviewed changes
src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_csv_env.php Outdated
{
private $parameters;
private $targetDirs = array();
private $privates = array();
Copy link
Member

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()
     {

Copy link
Member Author

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?

Copy link
Member

@nicolas-grekas nicolas-grekas Dec 29, 2017 •
edited
Loading

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.

@dunglas dunglas mentioned this pull request Dec 29, 2017
Closed
@javiereguiluz javiereguiluz changed the title [DI] Add a simple array env var processor [DI] Add a simple CSV env var processor Jan 3, 2018
@nicolas-grekas
Copy link
Member

@dunglas fabbot failure here

@nicolas-grekas
Copy link
Member

OH, in fact this CS fix has already been done on master AFAIK, so please just rebase to relaunch tests.

@ostrolucky
Copy link
Contributor

Can we rename to csv_row instead of csv? Someone might think this should parse csv string with newlines into array of arrays and submit bug reports. csv_row is less ambiguous and more future proof.

@nicolas-grekas
Copy link
Member

@ostrolucky "csv_row" looks like noisy boilerplate to me. Native's str_getcsv doesn't have "row" in its name btw.

@fabpot
Copy link
Member

fabpot commented Jan 19, 2018

Thank you @dunglas.

@fabpot fabpot closed this Jan 19, 2018
fabpot added a commit that referenced this pull request Jan 19, 2018
@fabpot
feature #25627 [DI] Add a simple CSV env var processor (dunglas)
d94fbce 
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/a

Add 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
@dunglas dunglas deleted the csv-env-processor branch January 19, 2018 07:26
@javiereguiluz javiereguiluz mentioned this pull request Jan 21, 2018
Closed
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 29, 2018
Closed
@merk merk mentioned this pull request Mar 3, 2018
Closed
@bobvandevijver bobvandevijver mentioned this pull request May 4, 2018
Closed
@fabpot fabpot mentioned this pull request May 7, 2018
Merged
@ro0NL ro0NL mentioned this pull request Jun 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@fabpot fabpot fabpot approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
DependencyInjection Feature Status: Reviewed
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
7 participants
@dunglas @javiereguiluz @nicolas-grekas @ostrolucky @fabpot @chalasr @carsonbot