PHP

[shanebonham]

Cobblestone (and newer parts of the CMS) uses the convention of code lines no longer than 120 characters, and comment lines no longer than 80 characters.

[jstayton]

• I think we should adhere to all of the PSR standards, including PSR-2 as our coding style.
  • Even in legacy projects like Monk CMS. If that means converting a file before work is done on it, I think it's better than having project-specific conventions that weren't really based on anything.
• Use PHP_CodeSniffer to format using PSR-2 and sniff out bad code.
• Use Composer for dependency management.
• Use phpDocumentor for code documentation.
• Use Codeception for app testing and phpUnit when only unit testing is necessary.
• Use Grunt for task running. This may seem odd, but it's being used more-and-more for PHP projects. There are number of Grunt plugins for PHP tools. And Phing sucks.
• Use analysis and code quality tools like:
  • PHPLOC
  • PHPCPD
  • PHPCDC
  • SensioLabs Security Checker
  • PHP Mess Detector

[mcordell]

Codeception runs phpUnit tests so there is no reason to require phpUnit or even really think about phpUnit other than to realize that tests written for phpUnit will work fine in Codeception.

[mcordell]

If we switch to Grunt could we start the process of automatic minification on assets?

[jstayton]

Codeception runs phpUnit tests so there is no reason to require phpUnit or even really think about phpUnit other than to realize that tests written for phpUnit will work fine in Codeception.

I mentioned phpUnit only for projects that don't require everything Codeception provides, like Monk ID PHP that only requires a unit test.

If we switch to Grunt could we start the process of automatic minification on assets?

Absolutely.

[shanebonham]

Are you proposing we abandon whatever reservations we have about PSR-2 and embrace it wholeheartedly? I'm thinking primarily about how it requires 4 spaces for indenting, or opening braces on the next line.

[jstayton]

Yes. Those two things you mentioned certainly make me cringe, but I think about it like this: Go had its code style defined from the beginning, and nobody thinks about going against it now. PHP didn't have that a few years ago, but most of the major players now agree on a code style. Why not embrace it now?

[shanebonham]

I don't disagree, I was just clarifying.

[mcordell]

Would the updates be pushed to the formatting branch, if so we might want to get on a more regular merge cycle for that branch?

[shanebonham]

I feel like as a matter of process, we should always merge formatting into dev/master whenever we do a deploy. Maybe worth adding to the deploy script?

[mcordell]

Something feels wrong about that, if a formatting change somehow broke functionality we probably wouldn't catch it until it hit production

[shanebonham]

That's what tests are for! :smile:

[shanebonham]

Even if we didn't auto-deploy it, I still think that is the right rhythm to get into.

I'm curious about what others think of the formatting branch. Is it too complicated of a process?

[rickyatmonk]

If there's a merge conflict would the deploy script just fail? Up to the deployer to resolve conflicts?

[jstayton]

I think it's over-complicated. If I start to work on a file that's unformatted, it's much easier to format + commit + start with my changes than deal with the formatting branch.

[shanebonham]

I agree that is easier. Does it make reviewing harder though? Maybe we should rather just deal with it on the PR side. That is, when you do your PR, exclude those commits and wrap them in a separate PR.

[mcordell]

Just as a note. Here is an example of what happens as you try to bring a file up to PSR-2 standards. I did this automatically using PHP_Beautifer and PHP_Code_Fixer. If anyone else is interested in setting up these tools lmk because there were some bugaboos with OSX.

Final file. Mostly passes PSR-2 except for some try-catch stuff and multiline functional calls.

What git blame looks like

See the commits to see what happens when you apply each tool. You need to apply beautifier first and then code fixer.

[shanebonham]

We may still find that we need to maintain our own 'flavor' of PSR-2 standard. For example, I was able to pretty easily clean up a file, but the class statement at the top, which looks like this:

class Module_Form_Page_Capture_Fields extends Module_Form_Page_Capture

throws these errors against PSR-2:

--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 3 | ERROR | Each class must be in a namespace of at least one level (a
   |       | top-level vendor name)
   |       | (PSR1.Classes.ClassDeclaration.MissingNamespace)
 3 | ERROR | Class name "Module_Form_Page_Capture_Fields" is not in camel caps
   |       | format (Squiz.Classes.ValidClassName.NotCamelCaps)
--------------------------------------------------------------------------------

The implication being that we'd need to rename classes, which seems dicey.

[jstayton]

I think it's OK if specific projects ignore specific PSR-2 rules, as long as those are documented within the project. New projects, however, should follow PSR-2 completely.

@mcordell Have you tried out the alpha of PHP_CodeSniffer? Wondering if you have any thoughts on how it compares to PHP_CS_Fixer for automatic fixing.

[mcordell]

I haven't, I was looking for their tool (phpcbf) and failed reading comprehension that it isn't released yet and must be in the alpha. I think ill try it out, no wonder Google was no help.

[mcordell]

So here is a quick comparison using event mapper (416 errors):

Using phpcbf (the alpha Codesniffer tool) and then re-running phpcs I was able to from 416->34 errors (382 fixed). Detailed result here Running phpcs on my fixed version above that used beautifer and CS_fixer I went from 416->50 errors(366 fixed). Detailed result here

It should be noted that phpcbf/alpha phpcs is still buggy. For instance I tried to apply phpcbf to my 50 error version above and it just wouldn't work because of some kind of bug.

[mcordell]

Testing convention question that arose out of getting codeception up and running with the forms module

[mcordell]

A sticking point with legacy code, PSR-2 compatibility, and pull requests arose recently. Our current practice is to fix PSR-2 violations on legacy files we are working on, commit that. Then we work on the file and commit the real "work". When pull request comes around this makes it harder on the reviewer because the differences are not easily ascertained, for example the PR above.

I purpose two possible solutions, which both rely on the same initial workflow (commit violation fixes separately from "real" work). However I think one of the two following options should be performed before a PR is given to a reviewer:

1. Separate the PSR-2 fixes into a separate PR by cherry-picking them onto dev. Get this PR approved and merged into dev and then rebase the 'real' work PR onto dev.
2. Through a rebase, place all of the PSR-2 commits at the beginning of the pull request so that the reviewer can use Github's diff tool to ignore them. E.g suppose we want to ignore the first commit of that PR we would visit the URL: https://github.com/MonkDev/mcms/compare/8c29976...add-pages-through-nav

Where the first part after "compare" is the hash of the first real work commit and the end is the end of the work commits.

1 is a little easier on the reviewer but requies a separate PR and might slow the review process down

[shanebonham]

I think I would vote 2, but also this is probably more of a general Git - Workflow issue, than specific to PHP (since any codebase may need conventions updates).

[jstayton]

Yeah. @mcordell, I think your issue is totally valid, but I don't think it's PHP-specific. I'd move it as @shanebonham suggested.