Coding Standard

[niden]

After discussion with @sergeyklay we are going to start enforcing a coding standard for Phalcon.

Main highlights:

• no underscore on properties
• camel casing
• use final as much as possible, except in cases that components can be extended
• use spaces vs tabs
• all methods MUST have a return value even if it is void
• avoid using "mixed typed" return types (string | int - bad int | long ok)
• test. code coverage.

For classes:

• Constants appear at the top of the class
• All constants sorted alphabetically based on constant name
• All properties appear based on their visibility as:
  • public
  • protected
  • private
• Properties are sorted alphabetical by name
• All methods appear based on their visibility as:
  • public
  • protected
  • private
• Methods are sorted alphabetical by name
• __construct() is always at the top of the class

More on this in our upcoming Hangout.

[niden]

We will be working on setting up some sort of checking similar to phpcs for Zephir and Phalcon.

[erisrayanesh]

Hi. generally it's great idea. I think most of classes should be extendable and to me, it's not clear exactly where are you going to use final? As I suggested in the issue 13873 for dividing method's routine into separated methods and subroutines as much as possible. Because it gives any class the maximum extendability.

[niden]

@erisrayanesh I agree with #13873 and that is the goal really. There is a lot of refactoring we need to do and the groundwork for this has started already. The results will of course not be visible from one version to another because we want to ensure that upgrades are as painless as possible.

For the final it will be wherever possible. For instance the Registry uses final methods. In any other component that does not need to be extended or should not be extended final will be used. For now just a guideline as we refactor slowly our components.

[Ultimater]

These looks like great standards. For clarify, when you say "no underscore on properties", do you mean no prefix underscores as well? Or just no snake_case in favor of camelCase?

[erisrayanesh]

@Ultimater If you check out the commits that @niden did, you'll find that standards also cover prefix underscores. Check the protected _reader property that converted to protected reader hear.

@niden Could you tell me that what if I need to extend the Registry for some minimal modification while it retrieves or stores the data? Actually I don't want to implement whole class methods.

[sergeyklay]

The biggest point of confusion and contention seems to be composition versus inheritance, often summarized in the mantra favor composition over inheritance.

[niden]

@erisrayanesh The Registry has always been final. However with the latest changes we pushed to v4, it is pretty much a wrapper around Collection you can then extend the Collection class which offers full read write.

@Ultimater What Arvand wrote. any property, method or variable prefixed with _ needs to be changed, and everything follows camel case.

If you think about it it this could very well be a PSR-2 standard for Zephir. Same rules apply for the most part

I have been working on this yesterday and although there is quite a bit of work left, I think I can finish it by the end of the day today. Then we can all move ahead with one way of doing things as far as Phalcon is concerned.

[dschissler]

I would also like PSR-4 style class file naming unless there is a Zephir reason why this is a bad idea. I've been doing it in my extension for half of a year without any problems. I changed to this because I found that when I was rapidly skipping back and forth between PHP and Zephir projects that I was needing to blink once before my brain could pattern match to a different style. After a day of programming it was just unnecessary cognitive load.

[dschissler]

Also I would like a loose case-by-case exception for writing tests for alpha versions regarding longer winded R&D. Maybe with a pledge to resolve any gaps in late alpha or early in the beta.

So for example I'll bring up two scenarios in which I felt the need and did not feel the need to immediately write tests for new features.

1) I added the ability to pass plain arrays to Config::merge. This is so simple of a change and so closed-ended that it would have been pure laziness to have not written a test then. Better to do it when ones head is still into the problem.

2) I was changing around the DI and PDO. I wasn't exactly sure where the development would lead me but I knew the general direction. I was able to get small PRs merged that didn't break anything and then to later continue working my way through it. I still as yet may or may not modify it further. I was also concerned that if my changes were to radical that my PR would sit for a very long time with just back and forth nonsense. So basically writing tests too early when nothing was breaking would have been a waste of my time, or at least I find it unpleasant enough that I'd rather not do it multiple times.

IMO very different scenarios and they should be treated differently. It would be nice to distil that down to just a few words without sounding too lawyery.

[joeyhub]

all methods MUST have a return value even if it is void

?

[Jeckerson]

@joeyhub

Example 1:

/**
  * @return void
  */

Example 2:

public function test() : void

[joeyhub]

That makes a lot more sense. I thought it meant return void or have to return some type.

Though PSR-joeyhub adopts the recommendation of noise elimination. Nothing should be decorative. It might be useful to consider applying PSR-joeyhub here.

Basically you're just repeating the type in the prototype, it's redundant. Types should only be expressed in annodoc where they can't be expressed in the language so it becomes optional. You should only put in a doc if you have something to say that's not already said. So if you're going to say this returns 0 in this case or something else in the other you put it in. Or if you have to return a mixed type because the language doesn't support it. Otherwise you're just making noise. If the type specified in the function name is exact and the function name is self explanitory then you're just gaming the numbers game by artificially inflating LOC. You're dashboard might look nice but real developers will raise an eyebrow.

Notice that javadoc doesn't ask you for the type and think about that.

If you mean always have a return type when a return type is known rather than php doc that's different. You should try to always have the return type on the prototype. Only failing that do you describe it in the annodoc.

[dschissler]

What about moving to // comments from block /* */ comments inside methods?

[dschissler]

Hard and Soft line lengths?

[niden]

https://docs.phalconphp.com/4.0/en/coding-standard

[niden]

Resolved

[rudiservo]

Hi sorry to be late for the conversation, but I have might have a case against against final or private in most parts of phalcon.

On the security side it makes sense for some part, but it will make things harder in other cases like prototyping with incubator, or simply overloading a phalcon class, access some of it's internal variables. In my case hacking on the model side just with plain php was a bit frustrating. and that's about it!

Underscoring or CamelCassing is not a problem, has long has you dont force it to lowercase for some reason in the middle of the code, keep case sensitivity.

Sorry for addressing this has well but Controller with camel case is treated with underscoring in the URL while the action is obviously Casesensitive, can we standardize this? it is rather confusing.

[dschissler]

Sorry for addressing this has well but Controller with camel case is treated with underscoring in the URL while the action is obviously Casesensitive, can we standardize this? it is rather confusing.

While I've not in practice encountered difficulty around this, I have observed a great many complaints about this over the years. For some reason or another this aspect of Phalcon is somewhat deficient in one of; the convention standards, documentation or the implementation. I am quite comfortable to assume that some things could be improved around this area due to my overall rough experiences with Phalcon .

but I have might have a case against against final or private in most parts of phalcon.

I've already discovered miserable problems with this, even before this new policy. Before I had pushed the Service work into the 4.0.x tree I was working with modifying the Service constructor. I tried to create a Service/SharedService class that simply inherited and removed the need to pass in a true for the second argument. WELL, the damn service class was locked down with a final and so piss on my idea. So basically there is this tiny window to interject into the Phalcon development process and then its all locked down.

SO THEN WHAT!? Any kind of push forward, independent research or just about any non-Team endeavor is shut down. "Hey, see you in 2 years" is a piss ass response and that is what is being dialed in for the future. Basically if the community doesn't peal back the the over zealous final crap now then they are stuck with it (in every single damn instance, across the board for every thing everywhere).

I can't help but feel (more of) an adversarial irritation towards the project for this. It basically feels like an open source software vendor lock in where you get dragged around by your nose ring and told what you can and cannot do. Wait until the next version before you can do a damn thing. Is anyone else aware of how limiting that this can be?

Until I'm informed otherwise or told that my code will run at "quantum speed" then I will be assuming that this is simply a way to lock down the API so that only the much aloof Phalcon Team can develop it. If its more difficult to develop with then maybe it will make up for some of their deficiencies.

[CameronHall]

I agree with not using final. Phalcon isn't an enterprise framework that's managed by a corporation that's funneling hundreds of man hours into it. Developers should have the freedom to change what they want in the framework (of course, with reason).

the damn service class was locked down with a final and so piss on my idea @dschissler makes a really good point and I can sympathize because I've encountered a similar problem.

If we're using final, we're locking people out and we're removing flexibility. Phalcon is no longer a modular and opinionated framework. I genuinely can't see the benefits of doing this.

Not to mention, nothing pisses me off more than when start looking into a bug and begin prototyping in PHP only to have to recompile Phalcon without final so I can actually overload the method.

This whole use final deal seems very much like a "this a good idea because someone else said so" kind of deal. There isn't enough justification.

I can't help but feel (more of) an adversarial irritation towards the project for this. It basically feels like an open source software vendor lock-in where you get dragged around by your nose ring and told what you can and cannot do. Wait until the next version before you can do a damn thing. Is anyone else aware of how limiting that this can be?

100%. I am a developer, I want to use Phalcon. I've found a bug. I want to overload the offending method and resolve my troubles. Oh, wait. It uses final. Suppose I have to wait until the next version 😠

[niden]

Just a clarification,

• use final as much as possible, except in cases that components can be extended

When we have a component and that is designed to be extended then of course we will not use final. Where a particular method cannot be extended or should not, then we will use final. If this ends up being 10-15 methods throughout the whole framework (see Registry) then that is fine.

That is the meaning of that bullet point.

https://docs.phalconphp.com/4.0/en/coding-standard

This is the current standard in the docs. As you will note I have a last update date up there to ensure that we inform people when we make changes. If anyone feels that a rule is missing or a rule is not correct etc. lets change it.

[sergeyklay]

Please take a look https://ocramius.github.io/blog/when-to-declare-classes-final/

[dschissler]

I once tried to extend Service to make a SharedService class and I hit this. I don't know why that has to be final. It was just getting in my way. I'd hate to see more of this just because someone couldn't conceive of a use during the framework development period. Is there really such a problem of people extending a class and forcing everyone to use that?