PHP Code Style

[ximex]

Hier sind die bisherigen CodeStyle-Richtlinien zu finden:
http://admidio.org/dokuwiki/doku.php?id=de:entwickler:programmierrichtlinien

Hier hab ich etwas gefunden von weit verbreiteten PHP CodeStyle Standards:
https://rwetzlmayr.github.io/php-the-right-way/
http://www.php-fig.org/

Die 2 Dokumente sind interessant:
http://www.php-fig.org/psr/psr-1/
http://www.php-fig.org/psr/psr-2/

Ich würde mir auch anschauen wie das mit automatischem Testing (hier mal bezogen auf CodeStyle) mittels Travis-CI geht:
http://docs.travis-ci.com/user/languages/php/

[ximex]

https://github.com/squizlabs/PHP_CodeSniffer
http://cs.sensiolabs.org/

[sirnone]

Hallo, hier mal meine Meinung dazu:
Ich würde wenn möglich auf solch umfangreiche todo's und Zusatzprogramme verzichten. Der Code von Admidio ist derzeit sehr übersichtlich und der zusätzliche Aufwand schreckt vor allem nicht so geübte Programmierer/Helfer ab.
genau für solche sind wir ja nun zu Github gewechselt, da sollten wir es ihnen nun nicht wieder schwerer machen an Admidio mitzuwirken.

ich persönlich bin auch froh, wenn ich den Code nur mal schnell vom Server hole und sofort programmieren kann - ohne mir gedanken zu machen, welche Zusatztools ich benötige. (schliesslich sitzt man nicht sehr oft am heimischen PC)

[ximex]

Ok ich versuchs nochmal etwas anders zu erklären:
Hier geht es nicht darum das sich die entwickler selbst die programme installieren und ausführen müssen, sondern das bei jedem commit/push zu github automatisch ein tool (travis ci) angestoßen wird um die änderungen zu prüfen und direkt mitteilt ob der code so in ordnung ist.

[ximex]

I have made some first tests: https://github.com/ximex/admidio/tree/code-style-fix-travis

[Fasse]

Bei zusätzlicher Software bin ich ganz bei Stefan. Wenn dies allerdings auf Serverseite läuft, dann gehts wieder. Allerdings sollten wir auch hier nicht die Hürde zum Checkin zu hoch machen. Man könnte aber mal mit ein paar einfachen Regeln anfangen.

[ximex]

Also so wie ich das will würde das alles nur auf Serverseite laufen und kein zusätzlicher Aufwand für die Entwickler sein. Travis CI gibt nur ein Test OK oder Test failed an Github weiter, welches das Ergebnis anzeigt. Durch klick auf das ergbnis kommt man zu Travis CI wo man sich die einzelnen Fehlermeldungen anschauen kann und so nachbessern kann. Es hindert nichts auch einen Commit mit Fehlern etc zu mergen etc. Sollte man halt nicht ;-)

Bsp:
leaflet-extras/leaflet-providers#138
Hier sieht man das die ersten 2 Commits noch einen Test fail verursachen. Die 2 weiteren Commits haben dann keine fehler mehr.

[Fasse]

ok, das klingt so ganz gut und würde der formalen Code Qualität in Admidio wahrscheinlich nützen.

Vielleicht fängt man erst einmal mit ein paar Prüfungen an, damit sich alle daran gewöhnen können und würde dann nach und nach weitere Regeln hinzufügen.

Würdest du @ximex das weiter verfolgen?

Ich würde in nächster Zeit ganz gerne den Fokus auf den Release der 3.0 setzen und darin dann meine Zeit investieren :)

[ximex]

Ja würd ich übernehmen. Muss mich da zwar selbst noch bissl einarbeiten aber den ersten POC hab ich ja schon.
Ich kümmere mich ja gerade allgemein um die Dinge die nicht gerade direkt den Code betreffen, aber es neuen Entwicklern vereinfachen soll mitzuarbeiten ;-)

[ximex]

Some questions about the prefered code style:
What do you like more?

if ($x === true) {
    echo 'test';
}

if($x === true) {
    echo 'test';
}

if ($x === true)
{
    echo 'test';
}

if($x === true)
{
    echo 'test';
}

The last one is the style i found mostly in the code.
I like the first style most (Javascript mostly used style)
The most php code style guides say the 3rd one.

Which do you prefer?

[ximex]

Look at the elseif part. i prefer elseif.
(code style from php.net (javascript like))

if ($a > $b) {
    echo "a is größer als b";
} elseif ($a == $b) {
    echo "a ist gleich groß wie b";
} else {
    echo "a ist kleiner als b";
}

if ($a > $b) {
    echo "a is größer als b";
} else if ($a == $b) {
    echo "a ist gleich groß wie b";
} else {
    echo "a ist kleiner als b";
}

[ximex]

require_once('system/common.php');

or

require_once 'system/common.php';

[ximex]

we should prefer the second one if possible.
https://php.net/manual/en/language.operators.comparison.php

if ($a == $b)

if ($a === $b)

[ximex]

There are some different style coding standards you can look here:

• https://rwetzlmayr.github.io/php-the-right-way/#stilregeln_f%C3%BCr_den_code
• https://github.com/squizlabs/PHP_CodeSniffer/tree/master/CodeSniffer/Standards

Here you can find some rules:
http://cs.sensiolabs.org/#usage
If we agree on some rules i could make the travis test for that an fix this code style bugs

[sirnone]

I think this is already defined:
http://www.admidio.org/dokuwiki/doku.php?id=de:entwickler:programmierrichtlinien

[Fasse]

if($x == true) {
    echo 'test';
}

or

if($x == true)
{
    echo 'test';
}

I dont know. The other two are no option. Until now we choose the second one. But the first one will reduce code. What do you think @sirnone ?

The elseif it my prefered style and we use until now.

[sirnone]

I prefer the second one, but this is mostly because i use it in all my code.
the same on elseif and else.
the reduction of code is no argument i think, because the size of the files and also the speed of the functions will be the same as far as i know.

[ximex]

if($x == true) {
    echo 'test';
}

or

if($x == true)
{
    echo 'test';
}

have the same size in bytes. the first one is only one line less.

@Fasse why don't you want a space between the if and the (
http://www.admidio.org/dokuwiki/doku.php?id=de:entwickler:programmierrichtlinien
Wer möchte kann 1 Leerzeichen zwischen ControlKeyword (if) und der Bedingung machen, um sie von Methodenaufrufen abzugrenzen. Dies ist aber jedem selbst überlassen.

back to this:

if ($a == $b)

if ($a === $b)

what about this? Everywhere i read you should use the second one if possible. (faster, stricter comparison)
Here is a table of the different behaviours:
https://php.net/manual/en/types.comparisons.php

[ximex]

next thing i found:

if($x == true and $y == false)

or

if($x == true && $y == false)

Do we prefer and and or or && and ||

[ximex]

http://cs.sensiolabs.org/#usage

Done/No Error found

PSR-0

• psr0

PSR-1

• encoding
• short_tag

PSR-2

• elseif
• function_call_space
• function_decleration
• indentation
• line_after_namespace
• lowercase_constant
• lowercase_keywords
• method_argument_space
• multiple_use
• parenthesis
• trailing_spaces
• visibility

Symfony

• double_arrow_multiline_whitespaces
• duplicate_semicolon
• extra_empty_lines
• new_with_braces
• object_operator
• phpdoc_params
• spaces_before_semicolon
• standardize_not_equal
• ternary_spaces

Todo/Discussion/Not Tested

PSR-2

• braces (i think you don't want it)
• eof_ending (discussion)
• linefeed (i think you don't want it)
• php_closing_tag (i think you don't want it)

Symfony

• concat_without_spaces (discussion; see: concat_with_spaces)
• empty_return (i think you don't want it)
• include (i think you don't want it)
• multiline_array_trailing_comma (i think you don't want it)
• namespace_no_leading_whitespace (we don't use namespaces yet)
• operators_spaces (discussion)
• remove_leading_slash_use
• remove_lines_between_uses
• return (i don't want it)
• single_array_no_trailing_comma
• spaces_cast (discussion)
• unused_use
• whitespacy_lines (discussion; i want it)

Contrib

• align_double_arrow (discussion)
• align_equals (discussion)
• concat_with_spaces (discussion; see: concat_without_spaces)
• multiline_spaces_before_semicolon
• ordered_use
• short_array_syntax (would be nice but only in PHP 5.4+)
• strict (discussion; i want it; much work to do)
• strict_param (discussion; i want it)

[ximex]

at combining strings i found different code styles:

$x = 'Stringstart'.'Stringend';
$x = 'Stringstart' . 'Stringend';
$x = 'Stringstart'.$y.'Stringend';
$x = 'Stringstart' .$y. 'Stringend';
$x = 'Stringstart' . $y . 'Stringend';

I prefer a space between every 'string' and $x and .

[Fasse]

if ($a == $b)

if ($a === $b)

This two phrases have different meanings. We should not target one with a commit check. People can use the second one but must not.

Until now we use '&&' and '||'.

At last I prefer $x = 'Stringstart'.$y.'Stringend'; but the last one could also be possible.

By the way @ximex you have now admin rights :)

[ximex]

ok because i found also and and or in the code. the dangerous thing here is the priority (https://php.net/manual/de/language.operators.precedence.php)

then i will change all string joining with spaces between every string, dot and variable if it is not your codestyle without any space

i know that == and === have different behaviours. but anyway i would change nerarly all == to === because of two reasons:

• faster (i also did some other performace improvments like change strlen($x) == 0 to $x === '')
• saver/stricter tests (if i want to know if it's the number 5 i don't want a true if it's '5')

[ximex]

CodeStyle Check is active now!
https://travis-ci.org/Admidio/admidio

looking forward to change the email notifications (today only me got notified) and to make the tests stricter over the time

[Fasse]

But Now wie should Do no changes to the Beta. Changes like === Could lead to new Problems . Sometimes i want 5 === '5'

[Fasse]

@ximex Is it possible to revert changes to ===. This could lead and has lead to problems. If you do a mass change to this than you don't know where it get to problems.

Please have a look at the two bugs that causes from this mass changes.

[ximex]

issue #69 comes from a little other code optimization.
old way: strlen($x) > 0
new way: $x !== ''

if $x is '' (empty string) there is no problem
but if $x is null than strlen is also false but !== '' is true.
i only have to change all null to '' (empty string)

Every change could lead the a new bug but this doesn't mean that the change is bad idea. i only have to fix the new bug ;-)
I will look at the 2 reported bugs

[Fasse]

ok, but then we must fix these bugs so that the master is usable :)

[ximex]

Question to SQL Code Style:
Should we write all SQL keywords in Uppercase letters?

Example:
From:

alter table %PREFIX%_users add constraint %PREFIX%_FK_USR_USR_CHANGE foreign key (usr_usr_id_change)
      references %PREFIX%_users (usr_id) on delete set null on update restrict;

To:

ALTER TABLE %PREFIX%_users ADD CONSTRAINT %PREFIX%_FK_USR_USR_CHANGE FOREIGN KEY (usr_usr_id_change)
      REFERENCES %PREFIX%_users (usr_id) ON DELETE SET NULL ON UPDATE RESTRICT;

[Fasse]

In my opinion YES. This is more readable if you don't have syntax highlighting.
What do you think?

[ximex]

i'm for upper case

[ximex]

the most things would get fixed in v3.1

[Fasse]

So we can Close this issue? If there are something new to discuss we can open a new issue for that Special Thing.

[ximex]

Plesse leave this still open. I will close it if i'm happy with the status

[Fasse]

Ok

[ximex]

here it is easy to see which fixer is activated:
https://github.com/Admidio/admidio/blob/master/.php_cs#L8-L97

[ximex]

Here are all PSR Standards: http://www.php-fig.org/psr/
these are standards they affect the codestyle:

• CodeStyle: PSR-1, PSR-2, PSR-12 [draft]
• Autoloading: PSR-4
• PHPDoc: PSR-5 [draft]

[ximex]

What do you like more?

// only if $xyz is always a bool !!!

// Current Version
if ($xyz == true)
if ($xyz == false)
// Change to:
if ($xyz === true)
if ($xyz === false)
// OR (my preferred version)
if ($xyz)
if (!$xyz)

[ximex]

no i'm happy with the code. i think i will find some more small things but the could get also changed after this issue is closed. for bigger changes i will open a specific issue