Skip to content

Fixed the third argument in VarDumperTestTrait.#2070

Merged
TomasVotruba merged 1 commit intorectorphp:masterfrom
adrozdek:fix/var-dumper-test-trait-argument
Oct 4, 2019
Merged

Fixed the third argument in VarDumperTestTrait.#2070
TomasVotruba merged 1 commit intorectorphp:masterfrom
adrozdek:fix/var-dumper-test-trait-argument

Conversation

@adrozdek
Copy link
Copy Markdown
Contributor

@adrozdek adrozdek commented Oct 1, 2019

In VarDumperTestTrait's asserts the third argument is called $filter and by default it should be equal to 0.
Right now it's called $context or $format and is set to null. Also $data argument is called $format sometimes.

public function assertDumpEquals($expected, $data, $filter = 0, $message = '')
public function assertDumpMatchesFormat($expected, $data, $filter = 0, $message = '') -
https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/VarDumper/Test/VarDumperTestTrait.php

@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented Oct 2, 2019

This rule is in Symfony 4.0 set. You address Symfony 4.4

When was this changed? Link to UPGRADE.md or PR in Symfony is needed. Maybe we need new rules to Symfony 4.4, we cannot change old ones

@adrozdek
Copy link
Copy Markdown
Contributor Author

adrozdek commented Oct 3, 2019

Sorry for giving 4.4 version but this parameter is the same for 4.0: https://github.com/symfony/symfony/blob/4.0/src/Symfony/Component/VarDumper/Test/VarDumperTestTrait.php
The UPGRADE-4.0.md for VarDumper is wrong. I created PR for changing that: symfony/symfony#33821

The description is wrong but the example taken from code is correct:
Zrzut ekranu z 2019-10-03 20-40-57

@TomasVotruba
Copy link
Copy Markdown
Member

I see, thanks for clarification

@TomasVotruba TomasVotruba merged commit 7226e7e into rectorphp:master Oct 4, 2019
TomasVotruba added a commit that referenced this pull request Apr 14, 2022
rectorphp/rector-src@d30a863 [Feature] Add configurable InlineSimplePropertyAnnotationRector for inlining of simple annotations (#2070)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants