Skip to content

Comments

Apply Doctrine CS 5.0#752

Merged
jwage merged 1 commit intodoctrine:masterfrom
Majkl578:cs-5.0
Sep 25, 2018
Merged

Apply Doctrine CS 5.0#752
jwage merged 1 commit intodoctrine:masterfrom
Majkl578:cs-5.0

Conversation

@Majkl578
Copy link
Contributor

Q A
Type improvement
BC Break no

120
)
;
);
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, is this change due to a rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep.

$process = new Process($runDoctrinePharCommand);

$process->start(function ($type, $buffer) use (&$output, &$successful) : void {
$process->start(static function ($type, $buffer) use ($successful) : void {
Copy link
Member

Choose a reason for hiding this comment

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

Does this still work with $successful not being used by reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted, I missed this one. Looks like a bug, will investigate once back on PC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixer bug reported here: slevomat/coding-standard#500, fixed fix manually.
Also removed unused $buffer which was not detected by any sniff. :/

@jwage
Copy link
Member

jwage commented Sep 25, 2018

Should we get #749 merged?

@Majkl578
Copy link
Contributor Author

Let's do this one first, I'll then rebase #749.

@jwage jwage merged commit c369ce8 into doctrine:master Sep 25, 2018
@jwage
Copy link
Member

jwage commented Sep 25, 2018

You da man 👍

@jwage jwage self-assigned this Sep 25, 2018
@Majkl578 Majkl578 deleted the cs-5.0 branch September 25, 2018 22:27
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