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

[DIC] Add a split env var processor #31867

Closed
wants to merge 1 commit into from

Conversation

Orkin
Copy link

@Orkin Orkin commented Jun 5, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#11674

This adds a new explode processor that will explode a string to a PHP array

@nicolas-grekas
Copy link
Member

I think you should use the csv processor instead.

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 5, 2019
@OskarStark
Copy link
Contributor

This feature should go in 4.4 branch instead of master, same for the corresponding docs PR please 🙏

@ro0NL
Copy link
Contributor

ro0NL commented Jun 6, 2019

Agree with @nicolas-grekas , you can already use the csv processor: https://3v4l.org/iGdpS

@Orkin Orkin changed the base branch from master to 4.4 June 7, 2019 13:18
@Orkin
Copy link
Author

Orkin commented Jun 7, 2019

@OskarStark base branch updated ;)

@OskarStark
Copy link
Contributor

@Orkin can you please give us some more insights, why you cannot/wont use the csv processor?

Thank you

@ro0NL
Copy link
Contributor

ro0NL commented Jun 14, 2019

i realized there can be different behavior :) https://3v4l.org/gOCPR

that's escaping support out-of-the box 😅 but perhaps that works counter-wise for @Orkin

for the csv processor itself, from the example above; im not sure why PHP doesnt unescape d\",\"e

So for the default parameter values "" and \" have the same meaning. (https://php.net/str_getcsv)

Perhaps SF should fix it?

@nicolas-grekas
Copy link
Member

@ro0NL same as in #32007? Worth a PR on the csv process? Please submit a PR if you think so.
But anyway, we're missing feedback from @Orkin before we can make guesses.

@ro0NL
Copy link
Contributor

ro0NL commented Jun 14, 2019

not sure if #32007 is related, as it fixes the encoding of bool(true) into "1". AFAIK Serializer doesnt decode "1" back into bool(true). Nor does PHP: https://3v4l.org/YckLo

For DI it's bool:key:N:csv:ENV IMHO.

If we decide to unescape the escape char, both DI + Serializer should do that yes. (Unless the escape char is ": https://3v4l.org/lgrDY)

https://bugs.php.net/bug.php?id=55413 is the bug report.

@Orkin
Copy link
Author

Orkin commented Jun 14, 2019

Hi, @OskarStark @nicolas-grekas first of all because I didn't know the csv processor. For my usecase it's for memcached server list retrieve via environment variable so for my need it's easier to split by something.
I choose a comma , but perhaps in the future it can be a space or a pipe |. It can be usefull to separate csv processor to a real flat array string and to get the availability to specify the separator.

I don't know as much env var processor so I don't have any ideas if it's possible and if not what kind of format it can be used. But in the future it can be a improvement.

And last thing, I don't want my string to be escaped because it can be url or something that need to stay in the same format

@OskarStark
Copy link
Contributor

In this case it makes sense to me to not use the csv processor and got with the new explode one. Also from a DX perspective, having csv:MEMCACHED_HOSTS... looks weird

@ro0NL
Copy link
Contributor

ro0NL commented Jun 14, 2019

Id prefer split then 👍

@OskarStark
Copy link
Contributor

Sounds good to me, but I think it’s more common to use explode kn the php context, also because of split is an old PHP 5.3 deprecated method

https://www.php.net/manual/en/function.split.php

But definitely better then using csv here 👍🏻

@ro0NL
Copy link
Contributor

ro0NL commented Jun 14, 2019

that's the thing, explode sounds PHP specific whereas in SF DI config it should be more language agnostic IMHO. Agree csv might look weird.

split by something. I choose a comma , but perhaps in the future it can be a space or a pipe |

i think having both split:ENV as well as split:|:CSV seems sensible 👍

@Orkin
Copy link
Author

Orkin commented Jun 14, 2019

It didn’t know and find the csv processor because it seems to me weird for my use case.
I’m agree with split because it’s a more common word but we are in a PHP world so most of the users of SF are PHP developers and when a PHP developer want to transform a string into an array the first function in his mind is explode that’s why it seems better to me.
We can imagine someone typing something like that in google how to explose an array in symfony yaml config

@ro0NL
Copy link
Contributor

ro0NL commented Jun 14, 2019

maybe csv is weird because of #25627 (comment), currently it accepts a single row, so it's somewhat split_escaped:ENV actually

i tend to believe people associate "CSV" with a collection of rows

explode vs. split: the latter is shorter.

@OskarStark
Copy link
Contributor

Let’s go with split then

@Orkin
Copy link
Author

Orkin commented Jun 14, 2019

Everything is up to date ;)

@OskarStark
Copy link
Contributor

Please update the PR title

@Orkin Orkin changed the title [DIC] Add a explode env var processor [DIC] Add a split env var processor Jun 15, 2019
@fabpot
Copy link
Member

fabpot commented Jul 8, 2019

Not sure we need yet another processor. csv works for most cases (even if not easily discoverable), and json works for more complex cases.

Here, I'm sure some will want to have another separator than ,, and others will want to be able to escape the separator.

So, I'm 👎 for this addition.

@nicolas-grekas
Copy link
Member

Closing as explained, thanks for proposing.

fabpot added a commit that referenced this pull request Sep 30, 2019
This PR was squashed before being merged into the 3.4 branch (closes #32051).

Discussion
----------

[Serializer] Add CsvEncoder tests for PHP 7.4

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? |no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Some CSV encoder tests to show the broken behavior of a trailing slash. Spotted in #31867, not sure what to do with it :)

Commits
-------

760354d [Serializer] Add CsvEncoder tests for PHP 7.4
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
hultberg pushed a commit to hultberg/symfony that referenced this pull request Sep 17, 2021
This PR was squashed before being merged into the 3.4 branch (closes symfony#32051).

Discussion
----------

[Serializer] Add CsvEncoder tests for PHP 7.4

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? |no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Some CSV encoder tests to show the broken behavior of a trailing slash. Spotted in symfony#31867, not sure what to do with it :)

Commits
-------

760354d [Serializer] Add CsvEncoder tests for PHP 7.4
@Orkin Orkin deleted the explode_env_processor branch December 20, 2021 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants