Coding-Standards: How to deal with new files and methods?

[sreichel]

Long story short ...

My proposals are:

1. use type hints for parameters and return type
2. use declare(strict_types = 1); for new classes
3. avoid underscore prefix for methods and properties
4. add @since <version> to docblock
5. for PR that are only for v20.0 add a note to docs blocks (makes it easier to compare to 19.4.x) - edit: outdated

[Flyingmana]

full support for 1,2,3

[kiatng]

I kinda like underscore prefix for protected methods and properties. But I can live without it.

[fballiano]

I think this has to be discussed in an RFC kinda way, my 2 cents:

1. ok for new methods, changing signature for existing methods creates breaking changes
2. I think it's ok but mixing strict types and non strict types, when a new class gets requires by an old... I don't know... I don't think it's 100% safe
3. for private/protected? mmmmm as @kiatng was saying I also like using the underscore
4. good idea
5. 100% against this one

[kiatng]

We need to decide on underscore prefix. PSR-12 states:

Method names MUST NOT be prefixed with a single underscore to indicate protected or private visibility. That is, an underscore prefix explicitly has no meaning.

Remove underscore prefix is breaking BC. Does PSR-12 address this issue? For me, it doesn't make sense to adopt this rule for OpenMage, it breaks consistency. Consistency is an important styling/design principle.

[sreichel]

@kiatng when old classes extend new ones ...

• if there are no typehints, nothing happens
• if methods use typehints, just make sure to match this (this is what is for?)

PSR-12 underscore ... we cannot remove them for BC, but we can avoid it for new properties and methods. Thats why i started #3482

[kiatng]

Question: can PHPMD check for PSR-12 on new classes and at the same time, not raise alarms for underscore prefix in old classes?

[sreichel]

Yes, but old classes have to be marked in DocBlock to supress this error.

[fballiano]

2 is about strict types, not type hints, that's where the mix worries me

[kiatng]

I am good for 1 ,2, 3, 4. Point 5 note on v20 is a bit of a hassle, if we have point 4 @since directive, point 5 may be is unnecessary?

I would also like to hear from other contributors: @addison74, @empiricompany, @elidrissidev, @kyrena, @luigifab, @colinmollenhour, @justinbeaty, @m-overlund, @pquerner, @Judx, @ma4nn, @dbachmann

[empiricompany]

I agree to go in this direction, although we need to be careful because errors can be easily created particularly with points 1 and 2.

Remove underscore prefix is breaking BC. Does PSR-12 address this issue? For me, it doesn't make sense to adopt this rule for OpenMage, it breaks consistency. Consistency is an important styling/design principle.

I also share the same opinion, I would keep the current coding style.

[ma4nn]

Yep, I also like points 1.-2.+4. 👍

Remove underscore prefix is breaking BC. Does PSR-12 address this issue? For me, it doesn't make sense to adopt this rule for OpenMage, it breaks consistency. Consistency is an important styling/design principle.

Although I don't like the underscore prefixing (and I don't use it in my OpenMage projects for custom code), I agree that consistency is more important here. Also thinking about some methods like _construct() or __() which are not so easy to migrate.

But I think lots of the discussions here relate to the question: what is the goal/vision of the OpenMage project?

1. Providing a stable Magento successor with reliable interfaces (unfortunately that is also protected methods in Magento) and a broad support for existing (old) Magento 1 Extensions or
2. Pushing the latest technologies (frontend & backend) and force merchants/users to use them and Extension providers to update their software.

If the statement in the README is still relevant ("maintaining a high level of backwards compatibility to the official releases"), then the "underscore question" should be easy to answer.

[m-overlund]

I do agree on this - well spoken.

[luigifab]

I think:

1. use type hints for parameters and return type: yes why not, how to add them for all methods?
2. use declare(strict_types=1); for new classes: no, add it all files after a long crash test, v22? 💣
3. avoid underscore prefix for methods and properties: I don't know, Magento 1 use it, and we can't rename all methods
4. add @ since to docblock: for new methods yes why not, but there are a lot of job to reprocess all new methods/properties

[elidrissidev]

I got pretty much the same comments:

1. Agree
2. Maybe, but needs extensive testing.
3. I would prefer to stay consistent with the rest of the codebase and keep the underscore for now.
4. Agree
5. No longer applicable

[sreichel]

I think:

1. _use type hints for parameters and return type:_ yes why not, how to add them for all methods?

2. _use declare(strict_types=1); for new classes:_ no, add it all files after a long crash test, v22? 💣

3. _avoid underscore prefix for methods and properties:_ I don't know, Magento 1 use it, and we can't rename all methods

4. _add @ since  to docblock:_ for new methods yes why not, but there are a lot of job to reprocess all new methods/properties

1. changing this for existing methods will break every extension, that uses rewrite . This would be only possible with rector rules. (WIP)
2. i'd make id mandatory for all new classes/methods. Changing existing one is huge work. (WIP)
3. i'd also keep it for existing ones, but do not indroduce new ones ... old ones should be marked with phpmd. It would be possible to change them too, via rector, but no ...
4. ... and we have to make sure this change is part of mentioned release ... leave @since TBD at least, do have it on track for later changes/correnctions.