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.

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

[4.0RFC] Remove "_underscore" from protected or private methods and properties #11766

Closed
slince opened this Issue Feb 27, 2018 · 19 comments

Comments

Projects
None yet
10 participants
@slince

slince commented Feb 27, 2018

Hi there!

  • bug
  • enhancement
  • feature-discussion (RFC)

I know this is a very old discussion first.

According to #4499, _protected() and __private() will be abondoned in version 4.0. but i did not find relevant item in this page 4.0-Roadmap

Is this missing?

@VarCI-bot VarCI-bot added the RFC label Feb 27, 2018

@dereuromark dereuromark added this to the 4.0.0 milestone Feb 27, 2018

@ADmad

This comment has been minimized.

Member

ADmad commented Feb 27, 2018

We have in general already stopped adding underscore prefix to new protected methods and properties in 3.x. Just haven't made a decision whether to go about removing underscore from methods/props in 4.0.

@slince

This comment has been minimized.

slince commented Feb 27, 2018

@ADmad In that case, there will be two styles of code

@ADmad

This comment has been minimized.

Member

ADmad commented Feb 27, 2018

I am not opposed to renaming the old protected methods/props if that's the consensus and someone is willing to take up the chore 🙂.

@slince

This comment has been minimized.

slince commented Feb 27, 2018

This can be done by regular replacement of phpstorm ... @ADmad

@markstory

This comment has been minimized.

Member

markstory commented Feb 27, 2018

Wouldn't this represent a significant break in compatibility for what is basically cosmetic reasons?

@inoas

This comment has been minimized.

Contributor

inoas commented Feb 27, 2018

Can't we wait with that in 5.x, deprecating and aliasing all of those in 4.x?

@saeideng

This comment has been minimized.

Member

saeideng commented Feb 27, 2018

removing old protected methods/props really not necessary
and we should focus on important things

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Feb 27, 2018

and we should focus on important things

❤️ 💯

@slince

This comment has been minimized.

slince commented Feb 28, 2018

@markstory

Wouldn't this represent a significant break in compatibility for what is basically cosmetic reasons?

We use the code style is to prevent the code is not beautiful, is not it?
Modifying protected and private properties does not break compatibility.

Can't we wait with that in 5.x, deprecating and aliasing all of those in 4.x?

There is a saying in China. One today is worth two tomorrows(明日复明日,明日何其多)

removing old protected methods/props really not necessary
and we should focus on important things

Coding style is not really the most important thing, but it's a basic;

There have been many discussions on this issue, and I think it should be time to solve; otherwise the problem will always be there, 3years or 5 years 😄

@mosaxiv

This comment has been minimized.

Contributor

mosaxiv commented Feb 28, 2018

I think it is better to fix it with 4.0 (better to be quick fix 😎)

With this problem, there is a time when cakephp beginners are confused when reading the code.

@markstory

This comment has been minimized.

Member

markstory commented Feb 28, 2018

Modifying protected and private properties does not break compatibility.

Except when it does. We have several protected 'hook' methods that are widely used in userland code. Removing prefixes from those hooks will certainly break code. This extends into all accessor methods in entities as well.

@slince

This comment has been minimized.

slince commented Feb 28, 2018

@markstory

Hi, according to https://semver.org/, you can do backwards incompatible changes if you change the major version.

@markstory

This comment has been minimized.

Member

markstory commented Feb 28, 2018

@slince Of course we can, but we have a goal of making the upgrade from 3 to 4 easy and not a giant headache for application developers. Large breaking changes like entity accessors that we don't currently have deprecation warnings or documentation for is not aligned with that goal.

@slince

This comment has been minimized.

slince commented Feb 28, 2018

@markstory I think you do not need to be so cautious. Too cautious does not necessarily benefit the development of the cake. Old school should be decisively removed, like 2.x to 3.x changes

😄

@ADmad

This comment has been minimized.

Member

ADmad commented Feb 28, 2018

The break in how entity accessor methods work would be a big headache. That's reason enough to not remove the prefix in 4.x.

@jeremyharris

This comment has been minimized.

Member

jeremyharris commented Feb 28, 2018

@slince I think the point is that we have learned our lesson from the big breaking changes between 2.x and 3.x. The lesson is that the many very large in the wild cake applications remain in 2.x because of the difficulty in upgrading. We are trying to prevent that issue in future major versions to help promote upgrading, while trying to strike the balance with BC and new features/practices.

@slince

This comment has been minimized.

slince commented Feb 28, 2018

It means that _protected() and __private() will always stay in cakephp ? or will be removed sometime in the future?

I believe many ordinary users will like me hope that they can be removed in the next major version.

@dereuromark

This comment has been minimized.

Member

dereuromark commented Feb 28, 2018

It is fair to assume that those will be gone in 5.x+ then. Just not at once, but piece by piece.
Closing then.

@markstory

This comment has been minimized.

Member

markstory commented Feb 28, 2018

It is fair to assume that those will be gone in 5.x+ then. Just not at once, but piece by piece.

I think not adding more new prefixed methods is a good way to minimize future breakage, and we can remove prefixes from non-user facing methods incrementally throughout 4.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment