8000 [3.0] Removed all $that variables by saro0h · Pull Request #12841 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[3.0] Removed all $that variables #12841

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
Dec 21, 2014
Merged

[3.0] Removed all $that variables #12841

merged 1 commit into from
Dec 21, 2014

Conversation

saro0h
Copy link
Contributor
@saro0h saro0h commented Dec 3, 2014
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ~
License MIT
Doc PR ~

@saro0h saro0h changed the title Removed all $that variables [3.0] Removed all $that variables Dec 3, 2014
@t 8000 imglabisch
Copy link

👍

@mayeco
Copy link
Contributor
mayeco commented Dec 3, 2014

👍

Thanks @saro0h

Ref: 2 sample

@fabpot
Copy link
Member
fabpot commented Dec 4, 2014

👍

@stof
Copy link
Member
stof commented Dec 4, 2014

We have a few methods which are marked as internal and are public only for such $that accesses. They should be switched to private

@saro0h
Copy link
Contributor Author
saro0h commented Dec 5, 2014

@stof I'm not sure to understand what you mean. Can you give an example so that I'll be able to do it?

@fabpot
Copy link
Member
fabpot commented Dec 5, 2014

@saro0h For preg_replace_callback calls, you need to pass an anonymous function, but sometimes, we use a method call. The method call should obvsiously be private but because of the way PHP works, we made them public with a note saying that it's really private (so, we tagged them with @internal -- not sure if it was done everywhere.) Now, we can just mark them private as they should be.

@jderusse
Copy link
Member
jderusse commented Dec 5, 2014

$that is not the only one variale used to replace $this in closures
some examples:
https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php#L66
https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/Fixtures/php/lazy_service.php#L49

And so many other ($self, $guesser, $test, ...) search the expression = $this;

@saro0h saro0h force-pushed the remove-useless-that branch 7 times, most recently from 357e7d3 to d63717e Compare December 9, 2014 22:23
@saro0h
Copy link
Contributor Author
saro0h commented Dec 9, 2014

Here you go (that was more work than I expected). Is that ok for you?

@saro0h saro0h force-pushed the remove-useless-that branch from d63717e to 3743b76 Compare December 9, 2014 22:27
jderusse
jderusse reviewed Dec 9, 2014
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Translation/PhpStringTokenParser.php
@@ -106,7 +106,7 @@ public static function parseEscapeSequences($str, $quote)
);
}

public static function parseCallback($matches)
private static function parseCallback($matches)
Copy link
Member

Choose a reason for hiding this comment

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

Can we change the visibility of a method who are not flagged as @internal? (even if it seem to be used only in closure)

Copy link
Member

Choose a reason for hiding this comment

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

It's meant to be merged into 3.0 where BC breaks are allowed.

@saro0h saro0h force-pushed the remove-useless-that branch from 3743b76 to 2c3e708 Compare December 16, 2014 22:39
fabpot
fabpot reviewed Dec 18, 2014
View reviewed changes
src/Symfony/Component/Console/Helper/ProcessHelper.php
* @internal
*/
public function escapeString($str)
private function escapeString($str)
Copy link
Member

Choose a reason for hiding this comment

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

What about inlining this one? Any other ones that could be inlined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is used 4 times in this class. Let me check in the other 39 files

8000
@saro0h saro0h force-pushed the remove-useless-that branch from 2c3e708 to 7b33d1a Compare December 21, 2014 12:11
@saro0h
Copy link
Contributor Author
saro0h commented Dec 21, 2014

Changes have been made

@fabpot
Copy link
Member
fabpot commented Dec 21, 2014

Thank you @saro0h.

@fabpot fabpot merged commit 7b33d1a into symfony:master Dec 21, 2014
fabpot added a commit that referenced this pull request Dec 21, 2014
@fabpot
minor #12841 [3.0] Removed all $that variables (saro0h)
ad8289b 
This PR was merged into the 3.0-dev branch.

Discussion
----------

[3.0] Removed all $that variables

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

Commits
-------

7b33d1a Removed all $that variables
@dosten dosten mentioned this pull request Apr 15, 2015
Merged
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.
7 participants
@saro0h @timglabisch @mayeco @fabpot @stof @jderusse @xabbuh
0