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

Documented the useAttributeAsKey() method #5314

Merged
merged 3 commits into from
Oct 14, 2015

Conversation

javiereguiluz
Copy link
Member

Q A
Doc fix? no
New docs? yes
Applies to all
Fixed tickets #4509

Two comments:

  • I've reordered the list of methods (addDefaultsIfNotSet, requiresAtLeastOneElement, useAttributeAsKey) to display it alphabetically and because it's better to have useAttributeAsKey at the end, just before showing the example.
  • I've left the XML code blocks empty and I'd like some XML expert to help me fill them.


$rootNode
->children()
->arrayNode('parameters')
Copy link
Member

Choose a reason for hiding this comment

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

missing ->fixXmlConfig('parameter') before line 219

@wouterj
Copy link
Member

wouterj commented May 25, 2015

I think you got confused by the config tree :) Let's show some examples:

Basic Prototyped Array (list)

$node
    ->fixXmlConfig('driver')
    ->children()
        ->arrayNode('drivers')
            ->prototype('scalar')->end()
        ->end() // drivers
    ->end()
;
drivers: ['mysql', 'sqlite']
<driver>msyql</driver>
<driver>sqlite</driver>

Output after processing:

Array(
    [0] => 'mysql'
    [1] => 'sqlite'
)

Prototyped Array With Children (list)

$node
    ->fixXmlConfig('connection')
    ->children()
        ->arrayNode('connections')
            ->prototype('array')
                ->children()
                    ->scalarNode('table')->end()
                    ->scalarNode('user')->end()
                    ->scalarNode('password')->end()
                ->end()
            ->end()
        ->end() // connections
    ->end()
;
connections:
    - { table: symfony, user: root, password: ~ }
    - { table: foo, user: root, password: pa$$ }
<connection table="symfony" user="root" password="null" />
<connection table="foo" user="root" password="pa$$" />

Output after processing:

Array(
    [0] => Array(
        [table] => 'symfony'
        [user] => 'root'
        [password] => null
    )
    [1] => Array(
        [table] => 'foo'
        [user] => 'root'
        [password] => 'pa$$'
    )
)

Prototyped Array (list) With Wrong Usage

Assume same tree as previous example, but now this yaml:

connections:
    sf_connection:
        table: symfony
        user: root
        password: ~
    default:
        table: foo
        user: root
        password: pa$$

The outcome will be exactly the same as the previous example, as the config component still handles connections as a list.

Prototyped Array (map)

Now, we can fix it using ->useAttributeAsKey():

$node
    ->fixXmlConfig('connection')
    ->children()
        ->arrayNode('connections')
            ->prototype('array')
                ->useAttributeAsKey('name')
                ->children()
                    ->scalarNode('table')->end()
                    ->scalarNode('user')->end()
                    ->scalarNode('password')->end()
                ->end()
            ->end()
        ->end() // connections
    ->end()
;

Yaml is the same as previous example.

<connection name="sf_connection"
    table="symfony" user="root" password="null" />
<connection name="default"
    table="foo" user="root" password="pa$$" />

Output after processing:

Array(
    [sf_connection] => Array(
        [table] => 'symfony'
        [user] => 'root'
        [password] => null
    )
    [default] => Array(
        [table] => 'foo'
        [user] => 'root'
        [password] => 'pa$$'
    )
)

@javiereguiluz
Copy link
Member Author

@wouterj thanks a lot for your excellent and insightful explanation. I've reused your exact same example to explain the method. Thanks!

@javiereguiluz
Copy link
Member Author

I wonder if we can label this PR as finished instead of in progress


The output configuration will be exactly the same as before. In other words, the
``sf_connection`` and ``default`` configuration keys are lost. The reason is that
the Symfony Config component treats arrays as lists by default.
Copy link
Member

Choose a reason for hiding this comment

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

Is that actually true when using the YAML format? Isn't the useAttributeAsKey() method only necessary when using the XML format because XML itself does not know anything like hashes?

Copy link
Member

Choose a reason for hiding this comment

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

@xabbuh actually, there is an inconsistency in Symfony currently: if we need to merge lists coming from multiple files, the keys are lost (merging them in a new list indexed numerically). However, in case merging is not necessary (a single file configures this list), we currently don't reset keys

@stof
Copy link
Member

stof commented Jun 6, 2015

@javiereguiluz please also document the ->normalizeKeys() method and advocate the usage of ->normalizeKeys(false) for prototyped associative array nodes. Keys defined by the user should not have dashes normalized to underscores as it causes confusion.
The ->normalizeKeys() method was added in 2.2 to solve this issue because of the design mistake in 2.0. But ideally, it would not exist and we would automatically skip the dash-to-underscore normalization for prototyped nodes.

.. code-block:: yaml

drivers: ['mysql', 'sqlite']

Copy link
Contributor

Choose a reason for hiding this comment

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

is this also possible?

drivers:
  - mysql
  - sqlite

Copy link
Member

Choose a reason for hiding this comment

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

yes it is, because this is exactly the same in YAML, so the Config component receives the same input.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but we don't want to write this example explicitly, too?

Copy link
Member

Choose a reason for hiding this comment

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

I see no need in doing that, this chapter is teaching you the Config component, not the Yaml syntax :)

@wouterj
Copy link
Member

wouterj commented Jul 28, 2015

@javiereguiluz do you maybe have time to finish this PR?

@weaverryan weaverryan merged commit 9fe9020 into symfony:2.3 Oct 14, 2015
weaverryan added a commit that referenced this pull request Oct 14, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

Documented the useAttributeAsKey() method

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes
| Applies to    | all
| Fixed tickets | #4509

Two comments:

  * I've reordered the list of methods (addDefaultsIfNotSet, requiresAtLeastOneElement, useAttributeAsKey) to display it alphabetically and because it's better to have `useAttributeAsKey` at the end, just before showing the example.
  * I've left the XML code blocks empty and I'd like some XML expert to help me fill them.

Commits
-------

9fe9020 Fixed a minor syntax issue
e77c3b2 Rewritten the explanation about the useAttributeAsKey() method
0f8f9fd Documented the useAttributeAsKey() method
weaverryan added a commit that referenced this pull request Oct 14, 2015
@weaverryan
Copy link
Member

Merged and continued in #5787

weaverryan added a commit that referenced this pull request Oct 15, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

Definition Tweaks - see #5314

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets | n/a

See #5314 - I am quite a novice with the Configuration component :).

Commits
-------

51bc906 Fixes thanks to Stof
4e788e4 fixing build failure
67b8ff2 Tweaks - see #5314
weaverryan added a commit that referenced this pull request Oct 15, 2015
* 2.3:
  changing includes to links
  Updating book examples to not use deprecated validation methods
  Updated constraint reference docs for Null->IsNull, False->IsFalse, True->IsTrue
  Creating placeholder constraint rst docs for deprecated usage
  Renaming constraint rst files to Is* to preserve edit history
  Fixes thanks to Stof
  fixing build failure
  Tweaks - see #5314
weaverryan added a commit that referenced this pull request Oct 15, 2015
* 2.7:
  tweaks for 2.7
  Fixing build error
  Tweaks thanks to everyone!
  [WIP] Reworking most of the registration form:
  Fixing build problems and wrong links
  changing includes to links
  Updating book examples to not use deprecated validation methods
  Updated constraint reference docs for Null->IsNull, False->IsFalse, True->IsTrue
  Creating placeholder constraint rst docs for deprecated usage
  Renaming constraint rst files to Is* to preserve edit history
  Fixes thanks to Stof
  fixing build failure
  Tweaks - see #5314
weaverryan added a commit that referenced this pull request Oct 15, 2015
* 2.8: (29 commits)
  Removing getName() as it's not needed in 2.8
  tweaks for 2.7
  Fixing build error
  Tweaks thanks to everyone!
  [WIP] Reworking most of the registration form:
  Fixing build problems and wrong links
  Fixing build error
  Rename CollectionType entry options
  changing includes to links
  Updating book examples to not use deprecated validation methods
  Updated constraint reference docs for Null->IsNull, False->IsFalse, True->IsTrue
  Creating placeholder constraint rst docs for deprecated usage
  Renaming constraint rst files to Is* to preserve edit history
  Fixes thanks to Stof
  fixing build failure
  Tweaks - see #5314
  Minor tweaks
  Final changes
  Reworded the introduction and other minor fixes
  Added a note about the advantages/drawbacks of listeners/subscribers
  ...
@javiereguiluz javiereguiluz deleted the fix_4509 branch May 24, 2018 15:59
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.

6 participants