8000 [Notifier] Only use sprintf instead of sprintf and string concat by OskarStark · Pull Request #39412 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Notifier] Only use sprintf instead of sprintf and string concat #39412

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
Dec 9, 2020

Conversation

OskarStark
Copy link
Contributor
Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? no
Tickets ---
License MIT
Doc PR ---

@@ -24,7 +24,7 @@ public function __construct(string $message, string $dsn = null, ?\Throwable $pr
{
$this->dsn = $dsn;
if ($dsn) {
$message = sprintf('Invalid "%s" notifier DSN: ', $dsn).$message;
$message = sprintf('Invalid "%s" notifier DSN: %s', $dsn, $message);
Copy link
Member

Choose a reason for hiding this comment

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

this was on purpose, because fabbot used to scream at %s that are not surrounded by double quotes.
Not sure why it doesn't scream anymore at these.
This should be double checked before merging!

Copy link
Member

Choose a reason for hiding this comment

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

oh, I can answer myself ...:P
this is not in a throw expression, that's why.

Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

this is a CS change, it should be on 5.1 to prevent issues with merges.
another possibility is to reject it.

@@ -24,7 +24,7 @@ public function __construct(string $message, string $dsn = null, ?\Throwable $pr
{
$this->dsn = $dsn;
if ($dsn) {
$message = sprintf('Invalid "%s" notifier DSN: ', $dsn).$message;
$message = sprintf('Invalid "%s" notifier DSN: %s', $dsn, $message);
Copy link
Member

Choose a reason for hiding this comment

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

oh, I can answer myself ...:P
this is not in a throw expression, that's why.

@nicolas-grekas nicolas-grekas dismissed their stale review December 9, 2020 14:19

see new comment

@OskarStark
Copy link
Contributor Author

this is not in a throw expression, that's why.

Ok

@nicolas-grekas
Copy link
Member

Thank you @OskarStark.

@nicolas-grekas nicolas-grekas merged commit 56f0456 into symfony:5.1 Dec 9, 2020
@OskarStark OskarStark deleted the sprintf branch December 9, 2020 20:05
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.

3 participants
0