Skip to content

Do not mark injected properties as private when moved to constructor#1711

Merged
TomasVotruba merged 1 commit intorectorphp:masterfrom
holtkamp:dev-patch-inject-from-protected-var
Jul 9, 2019
Merged

Do not mark injected properties as private when moved to constructor#1711
TomasVotruba merged 1 commit intorectorphp:masterfrom
holtkamp:dev-patch-inject-from-protected-var

Conversation

@holtkamp
Copy link
Copy Markdown
Contributor

@holtkamp holtkamp commented Jul 9, 2019

As suggested in #1400 (comment)

@TomasVotruba
Copy link
Copy Markdown
Member

What about classes with no children?

@holtkamp
Copy link
Copy Markdown
Contributor Author

holtkamp commented Jul 9, 2019

As mentioned here #1400 (comment), I do not think "detecting" this is the responsibility of this rector...? Better to have a lot of small and focussed rectors and let them work together, right?

@TomasVotruba
Copy link
Copy Markdown
Member

Great approach and usually I'd agree.

I try to look at this from users perspective - what would they change in code at one go?
Would they not care about visibility?

@holtkamp
Copy link
Copy Markdown
Contributor Author

holtkamp commented Jul 9, 2019

Well, from my user-perspective: in case I have marked my property as public or protected, this has a reason...

Of course, in case rector can determine properly that the visibility can be set to private, this would be great. However: I do not know how to determine that reliable, so I choose the easy way 😄

In case it is possible for rector to determine whether:

  • a public property is used by by other code
  • a protected property is used by specializing classes

=> then this could be added. But I "assumed" this would be possibly get quite complex?

@TomasVotruba
Copy link
Copy Markdown
Member

I see. Thanks for explaning your point of view. It helped me to improve my pespective :)

Let's try your idea and see how it goes.

@TomasVotruba TomasVotruba merged commit 23c8c72 into rectorphp:master Jul 9, 2019
@TomasVotruba
Copy link
Copy Markdown
Member

Thanks for the PR!

@holtkamp holtkamp deleted the dev-patch-inject-from-protected-var branch July 9, 2019 13:56
TomasVotruba added a commit that referenced this pull request Jan 21, 2022
rectorphp/rector-src@cb96224 [DowngradePhp70][Transform] Add #[\ReturnTypeWillChange] on Downgrade + transform ArrayObject::getIterator() to keep working on php 8.1 (#1711)
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