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

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

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@dunglas
Member

dunglas commented Dec 29, 2017

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

This comment has been minimized.

Member

javiereguiluz commented Dec 29, 2017

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

@nicolas-grekas

Nice :)

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

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 29, 2017

(fixture needs an update to make tests green)

@dunglas

This comment has been minimized.

Member

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?

{
private $parameters;
private $targetDirs = array();
private $privates = array();

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 29, 2017

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

This comment has been minimized.

@dunglas

dunglas Dec 29, 2017

Member

But then it will fail for master right?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 29, 2017

Member

I'm not sure what you mean: this is for master already.

@javiereguiluz javiereguiluz changed the title from [DI] Add a simple array env var processor to [DI] Add a simple CSV env var processor Jan 3, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jan 3, 2018

@dunglas fabbot failure here

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jan 3, 2018

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

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Jan 6, 2018

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

This comment has been minimized.

Member

nicolas-grekas commented Jan 7, 2018

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

dunglas added some commits Dec 29, 2017

@fabpot

fabpot approved these changes Jan 19, 2018

@fabpot

This comment has been minimized.

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

feature #25627 [DI] Add a simple CSV env var processor (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/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 dunglas:csv-env-processor branch Jan 19, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment