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

[Config] allow changing the path separator #22253

Merged
merged 1 commit into from Jan 21, 2018

Conversation

Projects
None yet
6 participants
@bburnichon
Contributor

bburnichon commented Apr 3, 2017

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

In bundles configuration, no dots are allowed in key names, but as a standalone config, there is no such limitation that should be enforced.

By using current NodeBuilder it should be possible to change path separator used (for example: /)

@bburnichon bburnichon changed the title from Config allow dots in node names to [Config] allow dots in node names Apr 4, 2017

@bburnichon bburnichon changed the base branch from 2.7 to master Apr 4, 2017

@nicolas-grekas nicolas-grekas modified the milestone: 3.4 Apr 10, 2017

@nicolas-grekas nicolas-grekas changed the title from [Config] allow dots in node names to [Config] allow changing the path separator Jul 12, 2017

@bburnichon bburnichon changed the base branch from master to 3.4 Jul 12, 2017

@ogizanagi ogizanagi self-requested a review Sep 25, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 9, 2017

Member

Rebase needed (we don't merge PRs when they contain merge commit)

Member

nicolas-grekas commented Oct 9, 2017

Rebase needed (we don't merge PRs when they contain merge commit)

@bburnichon

This comment has been minimized.

Show comment
Hide comment
@bburnichon

bburnichon Oct 9, 2017

Contributor

Tests are failing on Form Component which I did not change. Is it a known issue?

Contributor

bburnichon commented Oct 9, 2017

Tests are failing on Form Component which I did not change. Is it a known issue?

@nicolas-grekas

(tests back to green)

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 9, 2017

Member

We will also need to update the UPGRADE-3.4.md (explain that implementing the interface without the added method is deprecated) and UPGRADE-4.0.md (mention the added method) files.

Member

xabbuh commented Oct 9, 2017

We will also need to update the UPGRADE-3.4.md (explain that implementing the interface without the added method is deprecated) and UPGRADE-4.0.md (mention the added method) files.

Show outdated Hide outdated UPGRADE-4.0.md
@xabbuh

xabbuh approved these changes Oct 10, 2017

@bburnichon

This comment has been minimized.

Show comment
Hide comment
@bburnichon

bburnichon Oct 10, 2017

Contributor

Twig and VarDumper failure don't seem to be related to my PR. Are failures expected?

Contributor

bburnichon commented Oct 10, 2017

Twig and VarDumper failure don't seem to be related to my PR. Are failures expected?

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 10, 2017

Member

The Twig failure should be fixable by rebasing on the latest 3.4 branch and the VarDumper tests are currently failing on the 3.4 branch too.

Member

xabbuh commented Oct 10, 2017

The Twig failure should be fixable by rebasing on the latest 3.4 branch and the VarDumper tests are currently failing on the 3.4 branch too.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 24, 2017

Member

Moving to 4.1 as 3.4 is now in beta, thus closed for new feats.

Member

nicolas-grekas commented Oct 24, 2017

Moving to 4.1 as 3.4 is now in beta, thus closed for new feats.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 24, 2017

@bburnichon bburnichon changed the base branch from 3.4 to master Oct 25, 2017

@bburnichon

This comment has been minimized.

Show comment
Hide comment
@bburnichon

bburnichon Oct 25, 2017

Contributor

Failures do not seem linked to my changes.

Contributor

bburnichon commented Oct 25, 2017

Failures do not seem linked to my changes.

@bburnichon

This comment has been minimized.

Show comment
Hide comment
@bburnichon

bburnichon Oct 30, 2017

Contributor

Rebased after merge of deprecation and removal of TreeBuilder::$builder

Contributor

bburnichon commented Oct 30, 2017

Rebased after merge of deprecation and removal of TreeBuilder::$builder

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 8, 2018

Member

@bburnichon please rebase: we don't merge PR with merge commits.

Member

nicolas-grekas commented Jan 8, 2018

@bburnichon please rebase: we don't merge PR with merge commits.

@bburnichon

This comment has been minimized.

Show comment
Hide comment
@bburnichon

bburnichon Jan 8, 2018

Contributor

Sorry I did update via github interface.

Contributor

bburnichon commented Jan 8, 2018

Sorry I did update via github interface.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jan 11, 2018

Member

I like this feature, but I wonder if "path separator" is the best name for this option. Path is strongly related to "file paths". Maybe "name separator" or "key separator" or "children separator" etc.

Member

javiereguiluz commented Jan 11, 2018

I like this feature, but I wonder if "path separator" is the best name for this option. Path is strongly related to "file paths". Maybe "name separator" or "key separator" or "children separator" etc.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 11, 2018

Member

@javiereguiluz "path" is the correct term, already used a lot in the component:

$ grep path src/Symfony/Component/Config/ -ri | wc -l
219
Member

nicolas-grekas commented Jan 11, 2018

@javiereguiluz "path" is the correct term, already used a lot in the component:

$ grep path src/Symfony/Component/Config/ -ri | wc -l
219
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas
Member

nicolas-grekas commented Jan 21, 2018

Thank you @bburnichon.

@nicolas-grekas nicolas-grekas merged commit 15e05e1 into symfony:master Jan 21, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jan 21, 2018

feature #22253 [Config] allow changing the path separator (bburnichon)
This PR was merged into the 4.1-dev branch.

Discussion
----------

[Config] allow changing the path separator

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22268
| License       | MIT
| Doc PR        |

In bundles configuration, no dots are allowed in key names, but as a standalone config, there is no such limitation that should be enforced.

By using current `NodeBuilder` it should be possible to change path separator used (for example: `/`)

Commits
-------

15e05e1 Add feature allowing change of Path Separator

@bburnichon bburnichon deleted the bburnichon:config-allow-dots branch Jan 24, 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