Skip to content

Updated the styles of the config parameters #15980

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
wants to merge 3 commits into from
Closed

Updated the styles of the config parameters #15980

wants to merge 3 commits into from

Conversation

javiereguiluz
Copy link
Member

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

This PR uses comment() which hasn't been merged yet. WIP PR at #15964

I have a question: is it mandatory that the output of this command is a valid YAML or XML file? If not, we can keep the changes in the title of the command. Otherwise, I'll revert those changes.

config_1

config_2

@javiereguiluz javiereguiluz mentioned this pull request Sep 28, 2015
Closed
60 tasks
@ogizanagi
Copy link
Contributor

I have a question: is it mandatory that the output of this command is a valid YAML or XML file? If not, we can keep the changes in the title of the command. Otherwise, I'll revert those changes.

That would be particular and maybe I'm wrong, but will using the stderr output to render anything but yaml output will allow to have both, while only getting the yaml output when piping it to a file / other streams ?

@javiereguiluz javiereguiluz changed the title [WCM] Updated the styles of the config parameters Updated the styles of the config parameters Sep 28, 2015
@javiereguiluz
Copy link
Member Author

@ogizanagi that could work ... but it looks like a hack to me, so I guess Symfony deciders will never accept it.

@fabpot
Copy link
Member

fabpot commented Oct 1, 2015

Yes, it should be be valid YAML.

@javiereguiluz javiereguiluz force-pushed the update_config_commands branch from ca56950 to a8de671 Compare October 1, 2015 13:53
@javiereguiluz
Copy link
Member Author

Reverted the wrong modifications and rebased.

@fabpot
Copy link
Member

fabpot commented Oct 1, 2015

Thank you @javiereguiluz.

@fabpot fabpot closed this Oct 1, 2015
fabpot added a commit that referenced this pull request Oct 1, 2015
@fabpot
minor #15980 Updated the styles of the config parameters (javiereguiluz)
56fa973 
This PR was squashed before being merged into the 2.8 branch (closes #15980).

Discussion
----------

Updated the styles of the config parameters

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

This PR uses comment() which hasn't been merged yet. WIP PR at #15964

**I have a question**: is it mandatory that the output of this command is a valid YAML or XML file? If not, we can keep the changes in the title of the command. Otherwise, I'll revert those changes.

![config_1](https://cloud.githubusercontent.com/assets/73419/10144197/9018a138-661c-11e5-8c9d-dc1b721533b8.png)

![config_2](https://cloud.githubusercontent.com/assets/73419/10144199/925e5604-661c-11e5-8597-8e01013c2e86.png)

Commits
-------

fdaa513 Updated the styles of the config parameters
Koc
Koc reviewed Oct 1, 2015
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Command/AbstractConfigCommand.php
@@ -26,7 +25,7 @@
*/
abstract class AbstractConfigCommand extends ContainerDebugCommand
{
protected function listBundles(OutputInterface $output)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removed typehint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we are doing weird things with the $output variable. Everything will be fixed once we settle on this: #16000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@javiereguiluz @ogizanagi @fabpot @Koc @carsonbot