Skip to content

[VarDumper] fix very special vars handling #13351

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
Merged
merged 1 commit into from
Jan 13, 2015
Merged

[VarDumper] fix very special vars handling #13351

merged 1 commit into from
Jan 13, 2015

Conversation

nicolas-grekas
Copy link
Member

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

@nicolas-grekas nicolas-grekas changed the title s[VarDumper] fix very special vars handling [VarDumper] fix very special vars handling Jan 9, 2015
@nicolas-grekas nicolas-grekas force-pushed the fix-vardumper branch 2 times, most recently from 40ef355 to 5ee47da Compare January 9, 2015 21:40
@nicolas-grekas
Copy link
Member Author

PR is ready. Fabbot is drunk and Travis will be fixed after subtree-splitting (for what is concerned by this)

stof
stof reviewed Jan 9, 2015
View reviewed changes
src/Symfony/Component/VarDumper/Tests/CliDumperTest.php
public function testSpecialVars56()
{
if (PHP_VERSION_ID < 50600) {
$this->markTestSkipped('PHP 5.6 or the symfony_debug extension is required.');
Copy link
Member

Choose a reason for hiding this comment

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

should you check for the extension being loaded to avoid skipping in this case ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the extension does not help here, I'm going to update the message

@stof
Copy link
Member

stof commented Jan 9, 2015

AFAICT, the PHP-CS-Fixer expects the & to be near the variable name (or with spaces on both sides), not near the assignment operator, which is why it adds the space everywhere.

for the indentation change at the end, I'm quite sure it is a bug in the PHP-CS-Fixer logic to detect closures, which is not taking into account the possibility to return by reference. I submitted PHP-CS-Fixer/PHP-CS-Fixer#964

@stof
Copy link
Member

stof commented Jan 9, 2015

@fabpot what is actually the Symfony coding standard for the placement of the & for references ? PSR-2 does not talk about spaces around operators at all. Given that Symfony says to put spaces around operators, the PHP-CS-Fixer is enforcing the spaces. It has no special rule for the placement of references after the assignment operator, which is why it requires a space there as well. But is the fixer right for that ?

@nicolas-grekas nicolas-grekas force-pushed the fix-vardumper branch 3 times, most recently from 2308390 to f3b2fe0 Compare January 10, 2015 15:56
@keradus
Copy link
Member

keradus commented Jan 11, 2015

It's a Operators should be arounded by at least one space. rule.
There is &= operator (T_AND_EQUAL), but no =&. It's in fact 2 operators.

@fabpot
Copy link
Member

fabpot commented Jan 13, 2015

Thank you @nicolas-grekas.

@fabpot fabpot merged commit e26dc2c into symfony:2.6 Jan 13, 2015
fabpot added a commit that referenced this pull request Jan 13, 2015
@fabpot
bug #13351 [VarDumper] fix very special vars handling (nicolas-grekas)
c6695d1 
This PR was merged into the 2.6 branch.

Discussion
----------

[VarDumper] fix very special vars handling

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

Commits
-------

e26dc2c [VarDumper] fix very special vars handling
@MisatoTremor MisatoTremor mentioned this pull request Jan 13, 2015
Closed
@nicolas-grekas nicolas-grekas deleted the fix-vardumper branch January 13, 2015 14:51
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nicolas-grekas @stof @keradus @fabpot