Skip to content

[Console] changed warning verbosity; fixes typo #27858

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

Merged
merged 1 commit into from
Jul 6, 2018

Conversation

adrian-enspired
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes*
Fixed tickets n/a
License MIT
Doc PR n/a
  • Tests pass, but I do not have an installation of MacOS to run tests on. Tests should be unaffected (the test is simply skipped on MacOS).

When a Console Command fails to change the process title on MacOS, a warning is issued to output. This warning is relevant to developers of Console applications, but to end users is largely meaningless and potentially confusing.

This PR changes the verbosity of the warning to "very verbose" so it does not interrupt normal usage.
I've also fixed a typo in the message ("get" vs. "set").

Copy link
Contributor

@ogizanagi ogizanagi left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks! But this should target the lowest branch where it happens, i.e 2.8 AFAIK.
So could you rebase & re-target your PR on 2.8 please? :)

@@ -220,7 +220,10 @@ public function run(InputInterface $input, OutputInterface $output)
if (function_exists('cli_set_process_title')) {
if (!@cli_set_process_title($this->processTitle)) {
if ('Darwin' === PHP_OS) {
$output->writeln('<comment>Running "cli_get_process_title" as an unprivileged user is not supported on MacOS.</comment>');
$output->writeln(
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we keep everything on a single line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k. wasn't sure about line lengths.

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

It makes sense to me. Thanks @adrian-enspired!

(and please, put the new code in a single line ... even if it's long. thanks!).

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Jul 6, 2018
@nicolas-grekas nicolas-grekas changed the title changed warning verbosity; fixes typo [Console] changed warning verbosity; fixes typo Jul 6, 2018
@chalasr
Copy link
Member

chalasr commented Jul 6, 2018

Thank you for this first contribution @adrian-enspired!

@chalasr chalasr merged commit 86c771a into symfony:master Jul 6, 2018
chalasr pushed a commit that referenced this pull request Jul 6, 2018
…enspired)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Console] changed warning verbosity; fixes typo

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

* Tests pass, but I do not have an installation of MacOS to run tests on.  Tests should be unaffected (the test is simply [skipped on MacOS](https://github.com/symfony/console/blob/master/Tests/Command/CommandTest.php#L345)).

When a Console Command fails to change the process title on MacOS, a warning is issued to output.  This warning is relevant to developers of Console applications, but to end users is largely meaningless and potentially confusing.

This PR changes the verbosity of the warning to "very verbose" so it does not interrupt normal usage.
I've also fixed a typo in the message ("get" vs. "set").

Commits
-------

86c771a changed warning verbosity; fixes typo
chalasr pushed a commit that referenced this pull request Jul 6, 2018
…enspired)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Console] changed warning verbosity; fixes typo

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

* Tests pass, but I do not have an installation of MacOS to run tests on.  Tests should be unaffected (the test is simply [skipped on MacOS](https://github.com/symfony/console/blob/master/Tests/Command/CommandTest.php#L345)).

When a Console Command fails to change the process title on MacOS, a warning is issued to output.  This warning is relevant to developers of Console applications, but to end users is largely meaningless and potentially confusing.

This PR changes the verbosity of the warning to "very verbose" so it does not interrupt normal usage.
I've also fixed a typo in the message ("get" vs. "set").

Commits
-------

86c771a changed warning verbosity; fixes typo
@chalasr
Copy link
Member

chalasr commented Jul 6, 2018

Backported on 2.8 in ddea90e

@adrian-enspired
Copy link
Contributor Author

adrian-enspired commented Jul 6, 2018

Seems the lowest branch was actually 2.7.

rebasing had a lot of conflicts, so I made a new branch + PR instead:
#27880

Should I close this in favor of that PR?

edit
Nevermind...!

I posted this before seeing the more recent comments. closing my other!

nicolas-grekas added a commit that referenced this pull request Jul 7, 2018
* 2.8:
  [Console] fix CS
  improve deprecation messages
  minor #27858 [Console] changed warning verbosity; fixes typo (adrian-enspired)
nicolas-grekas added a commit that referenced this pull request Jul 7, 2018
* 3.4:
  [Console] fix CS
  [OptionResolver] resolve arrays
  [TwigBridge] Fix missing path and separators in loader paths list on debug:twig output
  [PropertyInfo] Fix dock block lookup fallback loop
  [HttpFoundation] don't encode cookie name for BC
  improve deprecation messages
  minor #27858 [Console] changed warning verbosity; fixes typo (adrian-enspired)
  AppBundle->App.
  [DI] Fix dumping ignore-on-uninitialized references to synthetic services
nicolas-grekas added a commit that referenced this pull request Jul 7, 2018
* 4.0:
  [Console] fix CS
  [OptionResolver] resolve arrays
  [TwigBridge] Fix missing path and separators in loader paths list on debug:twig output
  [PropertyInfo] Fix dock block lookup fallback loop
  [HttpFoundation] don't encode cookie name for BC
  improve deprecation messages
  minor #27858 [Console] changed warning verbosity; fixes typo (adrian-enspired)
  AppBundle->App.
  [DI] Fix dumping ignore-on-uninitialized references to synthetic services
nicolas-grekas added a commit that referenced this pull request Jul 7, 2018
* 4.1:
  [Console] fix CS
  [OptionResolver] resolve arrays
  [TwigBridge] Fix missing path and separators in loader paths list on debug:twig output
  [PropertyInfo] Fix dock block lookup fallback loop
  [FrameworkBundle] Fixed phpdoc in MicroKernelTrait::configureRoutes()
  [HttpFoundation] don't encode cookie name for BC
  improve deprecation messages
  minor #27858 [Console] changed warning verbosity; fixes typo (adrian-enspired)
  AppBundle->App.
  [Workflow] Fixed BC break
  [DI] Fix dumping ignore-on-uninitialized references to synthetic services
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.

7 participants