8000 [Config][RFC] EnumNode removes values based on loose comparison · Issue #48913 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Config][RFC] EnumNode removes values based on loose comparison #48913

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
kiler129 opened this issue Jan 8, 2023 · 2 comments
Closed

[Config][RFC] EnumNode removes values based on loose comparison #48913

kiler129 opened this issue Jan 8, 2023 · 2 comments
Labels
Bug Config Status: Needs Review

Comments

@kiler129
Copy link
Contributor
kiler129 commented Jan 8, 2023 •
edited
Loading

Symfony version(s) affected

v2.1.0+

Description

In my use-case I have a node with enabled field, which can have 3 values:

  • true: always enable component
  • false: do not enable the component
  • null: enable if it's needed (dependent on rather complex logic, outside of the scope of config schema)

The issue is I cannot use booleanNode() as it is well... boolean.

How to reproduce

    ->children()
        ->enumNode('enabled')
            ->values([true, false, null])
            ->defaultNull()
    ->end()

While the code above "works", using bin/console config:dump-reference reveals that in fact the null is not an expected values:

 doctrine_changeset_marshaller:
     enabled:              null # One of true; false

Possible Solution

The issue stems from usage of array_unique() in two places:

symfony/src/Symfony/Component/Config/Definition/Builder/EnumNodeDefinition.php

Lines 13 to 19 in 8308aea

class EnumNodeDefinition extends ScalarNodeDefinition
{
private $values;
public function values(array $values)
{
$values = array_unique($values);

symfony/src/Symfony/Component/Config/Definition/EnumNode.php

Lines 13 to 19 in 8308aea

class EnumNode extends ScalarNode
{
private $values;
public function __construct($name, NodeInterface $parent = null, array $values = array())
{
$values = array_unique($values);

The issue is, \array_unique() compares values according to their string representation. I believe this is less than idea in the case of EnumNode. The \array_unique(), unlike functions like \in_array() does not support $strict parameter (and the discussion on that died in 2013). Thus, to really solve this issue a custom strict function would need to be used:

However, this does not cover empty string case. Given the EnumNode usage of the array where keys in the array aren't important, I think the solution would be to replace \array_unique() call with a function like:

function arrayUniqueStrict(array $values): array
{
    $unique = [];
    foreach($values as $k => $v) {
        if (!\in_array($v, $unique, true)) {
            $unique[$k] = $v;
        }
    }

    return $unique;
}

Additional Context

If the intention of EnumNode to be type-loose, then my suggested fix is obviously a no-go. In such a case I think it should be noted in the docs.
@kiler129 kiler129 added the Bug label Jan 8, 2023
@kiler129 kiler129 changed the title [Config] EnumNode removes null and empty string values [Config][RFC] EnumNode removes values based on loose comparison Jan 8, 2023
@fancyweb
Copy link
Contributor

We do use boolean and null values in our own core bundles (EnumNode values are therefore not restricted to scalar values) so I guess the case you describe should logically work. But since it never did for the past 10 years that would be a new feature I guess?

We could maybe simply remove the array_unique call and let the duplicated values be displayed several times in messages and configuration references dumps?

@stof
Copy link
Member
stof commented Jan 11, 2023

I agree with @fancyweb here. Let's remove the array_unique call in 6.3

@fancyweb fancyweb mentioned this issue Jan 12, 2023
Merged
@fabpot fabpot closed this as completed Jan 19, 2023
fabpot added a commit that referenced this issue Jan 19, 2023
@fabpot
feature #48957 [Config] Do not array_unique EnumNode values (fancyweb)
bcfa507 
This PR was merged into the 6.3 branch.

Discussion
----------

[Config] Do not array_unique EnumNode values

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | #48913
| License       | MIT
| Doc PR        | -

Using `array_unique` for the values doesn't have an impact on the core behavior of this node but it causes an issue with non-scalar values that get coerced to the same string. Since it never worked, we could apply this change safely on 6.3.

Commits
-------

c9d8b15 [Config] Do not array_unique EnumNode values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Config Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @stof @kiler129 @fancyweb @carsonbot
0