prettier / plugin-php Goto Github PK

I wanted to tackle #7, so I compared the implementation of the JS and python printers with the PHP one. I noticed that both JS and python rely heavily on calls to path.call, patch.each and so on, for example

https://github.com/prettier/prettier-python/blob/0e5c2a77ceebe2814707b7c61d1d003b1186b564/src/printer/index.js#L125

while in the PHP printer we're usually just calling to the internal function printNode, for example
https://github.com/prettier/prettier-php/blob/cd78cbb906aabeaed8c1e9588d7d89a6e89bae07/src/printer.js#L123

While fastPath seems to be used mostly for performance reasons (please correct me if I'm wrong), the call to print seems important because it calls the core function printGenerically which handles comments, for example.

Does this mean that we should refactor the printer to use path.call, path.each, ...? Since this would be a major change and I guess it should've come up earlier, I thought I'd open an issue before getting started 😉 Thanks!

input:

$test ? doSomething() : doSomethingElse();

output:

$test ? doSomething() : doSomethingElse()

From #45 (comment)

This came to mind when seeing the output:

The opening brace of a function should be on its own line according to PSR-12 (same as class methods).

(but the opening brace of a closure goes on the same line as the closing parens of the parameter list)

input:

// multi
// line
// comment
// test

output:

// multi// line// comment// test

input:

/**
 * Multi line
* test with wrong indentation
*/

output:

/**
 * Multi line
 */

input:

namespace \test\testing;

require "/test/file.php";
require_once TEST . "/test/file.php";

output:

namespace \test\testing;

require "/test/file.php"
require_once TEST . "/test/file.php"

Before/after

-    protected $fillable = ['title', 'description', 'requester_id', 'type', 'summary', 'proof', 'budget_id'];
+    protected $fillable
+        = ['title', 'description', 'requester_id', 'type', 'summary', 'proof', 'budget_id'];

(actually I don't know if this kind of message really helps you, I'm just reporting all the bugs I'm seeing, but tell me if it's not relevant.)

We need issue template 👍

require ('somefile.php');

vs

require 'somefile.php';

All popular framework use this form:
https://github.com/laravel/framework/search?l=PHP&q=require&type=&utf8=%E2%9C%93
https://github.com/symfony/symfony/search?utf8=%E2%9C%93&q=require&type=
https://github.com/yiisoft/yii2/search?l=PHP&q=require&type=&utf8=%E2%9C%93

Vote welcome.

1. ❤️ - First example
2. 👍 - Second example
3. 💯 Output as is

I think we should discuss the general procedure on how we handle the wish to add new options.

I started writing this as a comment to #118, but I'd like to discuss the "big picture" in a separate issue instead.

To me, #118 is a nice example to prove @evilebottnawi 's point in the discussion about opening braces (#107, #108):

I'm not against the option or not specifically this option, the only problem is that adding one option other will want more options, and each will complicate the maintenance of the project and deliver only problems

In this case, I'd advise against introducing a new option, because the number of options we'd have to support if we allow configuration of "this type of" styling decision.
Thinking about it from this standpoint, I'd even support your wish to revert the option we introduced about curly braces and add it back in when we've released a first stable version and the tool has been in use for a while - because then we have an actual community of user that will keep asking for specific options.
I'd also like to cite two comments by @vjeux which might offer guidance:

For prettier, I used a combination of my experience writing JavaScript for 10+ years, grepping through the millions of lines of the Facebook codebase to see which style was more popular, reading through various coding styles (eg Airbnb) and what are all the related eslint configs and finally feedback from people complaining that some pattern looked bad.
My guiding principle was for the code generated by prettier to pass code review as if it was written by a human. I also tried to make all the decisions be as less controversial as possible.

Also, one thing that really helped is to have someone to make the final decision. This way all the discussions could happen and were useful input but at the end of the day, only the things I wanted would go through.
I found that there are a handful of useful tactics to manage those discussions:

• Ask for specific snippets of code and try to find solutions for those. So many times people argue about very general things but in practice there are concrete fixes you can do for their use case.
• Explicitly say that you're not going to make a decision right now on certain issues. One thing that people pick up quickly is to engage in places where the people involve the project are responding. Then once in a while you can sort issues by upvote/comments and figure out what are the most painful things and do a deep dive.
• Avoid style trashing. Ideally, you should think deeply about each important style decision you are making and once you do it, actually commit to it. This way you can "close" the issue and whenever this subject comes up in the future say that you have reached a decision and are not looking to spend time thinking about it for now. This way you can focus on the things that still need to be fixed rather than going through the same discussions over an over again wasting everyone's time. This is hard to do in practice but if you can pull it off, it'll help a lot.

(See #59 (comment))

I'm gonna make a suggestion on how i could imagine we handle things, just to keep this issue actionable:

• Until our first release, we don't add any custom options and remain PSR-2 compatible.

After that:

• We only consider a new option if there is a considerable amount of requests to add it
• If necessary, we put the decision up to a vote after a short "cool-down" period

That would also mean that we don't open anything that involves adding options for votes just yet. Of course we can (and should) vote on difficult stylistic decisions that go into the first version.

What do you think?

/cc @mgrip

I couldn't find anything in the various PSR, but it seems acceptable to add trailing commas to arrays. Thoughts about making this the default?

This

/*********************
 * Some long comment *
 *********************/

is transformed into

// ********************
 * Some long comment *
 ********************

Quoting @nicoder:

As far as I can tell, both are PSR-2-compliant, and PSR-12 adds precisions because there was debate about that:

https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md#47-method-and-function-calls

for example PSR-12 lists this is valid:

$app->get('/hello/{name}', function ($name) use ($app) {
    return 'Hello ' . $app->escape($name);
});

but this would not be :

$app->get('/hello/{name}',
    function ($name) use ($app) {
        return 'Hello ' . $app->escape($name);
    }
);

I understand it this way:

• if one argument is defined starting on a new line, then all of the arguments must start on their own line
• but arguments may take up more than one line (independently of whether they start on their own line or not)

For example it may be nicer to see:

foo([
   $longExpression,
   $prettyLongExpression,   
]);

than:

foo(
    [
       $longExpression,
       $prettyLongExpression,   
    ]
);

Or nicer to see:

$db->Execute($sql, [
    $foo,
    $somewhatLongParameter,
]);

than:

$db->Execute(
    $sql,
    [
        $foo,
        $somewhatLongParameter,
    ]
);

1st off I want to be clear - I think the default should still be to follow PSR2-like formatting standards

With that in mind, from my own experience and now starting to look through other open-source php projects, it seems like one of the biggest differences in formatting standards is putting the opening brace for functions, classes, etc on a new line vs same line. I'm a little worried that not having this option might scare people off from using prettier/plugin-php.

I was thinking of starting to work on a PR for adding an option to do this (defaulting to the PSR2 format of putting it on a new line), but wanted to open for discussion first.

openingBraceNewLine = true (default)

class Test
{
  public function testMethod()
  {
  }
}

function testFunction()
{
}

openingBraceNewLine = false


class Test {
  public function testMethod() {
  }
}

function testFunction() {
}

Examples taken from php docs
input:

for ($i = 1, $j = 0; $i <= 10; $j += $i, print $i, $i++);

output:

for ($i = 1$j = 0; $i <= 10; $j += $iprint $i$i++) {

}

I've created a new branch (nikulis/prettier-php@7ee8034) to address this, but did not want to PR at the moment since it is based on the already-open #14.

The php break statement should accept an optional trailing integer denoting the number of nested control statements to break. Currently, any trailing integer is removed.

Input:

<?php

while(true) {
  while(true) {
    break 2;
  }
}

Current output:

<?php

while(true) {
  while(true) {
    break;
  }
}

There are a couple other quirks surrounding the use of the break statement that may or may not fall under the purview of this project to auto-correct and/or offer configurable parameters for:

1. Apparently, php also allows for parentheses around the trailing integer, making it look like a function call, e.g. break(2);. Could we automatically remove the parentheses so that all breaks follow the same convention?

2. break 1; could be collapsed to break;.

It seems according to PSR-12 that we should have
(int)$var not (int) $var.

After resolve glayzzle/php-parser#117 we can output die and exit. Should we output always exit as good pretty practice or not?

input:

function &passByReferenceTest()
{
  $a = 1;
  return $a;
}

class Test {
  public function &passByReferenceTest()
  {
    $a = 1;
    return $a;
  }
}

output:

  function passByReferenceTest() {
    $a = 1;
    return $a;
  }
  
  class Test
  {
  
    public function passByReferenceTest()
    {
      $a = 1;
      return $a;
    }
  }

Right now, the name of the npm lib is @prettier/php.

Thank you for your work!

input:

function returnTypeTest(): string {
  return 'hi';
}

class Test
{
  public function returnTypeTest(): string {
    return 'hi';
  }
}

output:

function returnTypeTest() {
  return 'hi';
}
 
class Test
{
  
    public function returnTypeTest()
    {
       return 'hi';
    }
 }

Currently

<?php

for ($i = 0; $i < 10; $i++) {
    // for body
}

is being turned into

<?php

for ($i = 0;$i < 10;$i++) {
    // for body
}

which seems to go against PSR-12.

Indentation is wrong when chaining methods and using callables.

Before/after:

     public function jsonSerialize()
     {
-        return collect(parent::jsonSerialize())->mapWithKeys(function ($value, $key) {
-            return [camel_case($key) => $value];
-        })->toArray();
+        return collect(parent::jsonSerialize())->mapWithKeys(
+            function ($value, $key) {
+                return [camel_case($key) => $value];
+            }
+        )->
+            toArray(
+
+        );
     }

Thanks for your work!

$a = "\"";
$b = '\'';

becomes

$a = """;
$b = ''';

First up, awesome work on this project, I'm really excited. I seem to recall there was some discussion about this for JavaScript but I can't find the issue.

Right now this converts

$foo = "hello\nworld"

into

$foo = "hello
world"

Personally I think it is nicer to leave the \n alone.

When running prettier with the --debug-check option (for testing), I get a whole lot of ast differences around node locations. I think all this means is that we have to strip out node locations in our ast-clean function? https://github.com/prettier/plugin-php/blob/master/src/clean.js#L5

I took the habit of aligning all the arrows in a array.
Phpstorm do that automatically, and I find that a lot clearer.

-        'description'  => 'description',
-        'excerpt'      => 'extrait',
-        'date'         => 'date',
-        'time'         => 'heure',
-        'available'    => 'disponible',
-        'size'         => 'taille',
-        'already_paid' => 'déjà payé',
-    ],
+        'description' => 'description',
+        'excerpt' => 'extrait',
+        'date' => 'date',
+        'time' => 'heure',
+        'available' => 'disponible',
+        'size' => 'taille',
+        'already_paid' => 'déjà payé'
+   ]

What do you think about that?

To align with the PSR2 standard, we need to use 4 spaces for indentation, rather than 2 (which is happening now). I believe we achieve this by changing the default value for the tabWidth option from 2 to 4. I'm unsure exactly how to override the default value for a global option, from a plugin - @czosel pointed me to prettier/prettier#3924, but I haven't been able to implement successfully yet.

input:

function pass_by_reference_test($x, &$a) {
  $a + 1;
}

output:

function pass_by_reference_test($x, $a) {
  $a + 1;
}

input:

class Test {
  public function test_pass_by_reference(&$x) {
    $x + 1;
  }
}

output:

class Test {
  public function test_pass_by_reference($x) {
    $x + 1;
  }
}

if (rand() > 0.5) return true;

becomes

if (rand() > 0.5) {
    return true
}

I'm not sure about this one, this is just a question of opinion. I understand why it formats the array, but I find it less clear that way.

-    protected $listen = [
-        'App\Events\Event' => [
-            'App\Listeners\EventListener',
-        ],
-    ];
+    protected $listen = ['App\Events\Event' => ['App\Listeners\EventListener']];

Thoughts?

Unlike JavaScript community, PHP has widely adopted code style already that's used by all the major framework vendors. Will this library be implementing those code styles? Or will you guys be doing your own thing?

Currently, PSR-2 (and therefore also PSR-1) is what everyone uses, but PSR-12 is currently under review as a replacement for PSR-2.

We have two syntax:

• Easy

$string = "Foo ${bar}";

• Complex (curly) syntax

$string = "Foo {$bar}";

Ref: http://php.net/manual/en/language.types.string.php

Should we:

1. Output encapsed string as is.
2. Output always into complex (curly) syntax.

Curlys in encapsed strings are not fully supported yet ("two {$b}s" vs. "two ${b}s";).

See glayzzle/php-parser#101

https://github.com/glayzzle/php-parser/blob/master/docs/AST.md#continue

Same here, I can understand why it made that, but I think this is not a good idea:

-        Route::prefix('api')
-             ->middleware('api')
-             ->namespace($this->namespace)
-             ->group(base_path('routes/api.php'));
+        Route::prefix('api')->middleware('api')->namespace($this->namespace)->group(
+            base_path('routes/api.php')
+        );

Input:

if (version_compare(PHP_VERSION, '7.1.0', '<')) {
    insteria_error('Minimum required PHP version is 7.1.0', 'Insteria Theme Requirement Checker');
}

$autoloaderPath = __DIR__ . '/vendor/autoload.php';

Output:

if (version_compare(PHP_VERSION, '7.1.0', '<')) {
    insteria_error('Minimum required PHP version is 7.1.0', 'Insteria Theme Requirement Checker');
}
$autoloaderPath = __DIR__ . '/vendor/autoload.php';

I think we should newlines as is, it is compatibility with PSR.
Also i'm interested in helping, right now i'm support css/scss/sass in prettier (https://github.com/prettier/prettier/graphs/contributors). You can invite me and i will help to improve output ⭐

Having installed prettier and @prettier/plugin-php globally, I cannot seem to get prettier to work on any php files, it always results in a syntax error.

Am I doing something wrong?

I've got a large PHP API codebase that I can run prettier on to help you all test. Can you point me in the right direction for how to do that? Maybe I can add something to the readme.

input:

function type_hinting_test(array $test, int $int_test, string $string_test = '') {
    return $int_test;
}

output:

function type_hinting_test($test, $int_test, $string_test = '') {
    return $int_test;
}

Input:

class MyClass
{
    function myFunction() 
    {
        // Nothing
    }
}

Output:

class MyClass
{
    public function myFunction() 
    {
        // Nothing
    }
}

I think it is bug, feedback welcome!

discussion from #71 - investigate if we can reuse the logic from core prettier around comments

I think we should right now resolve this issue, at the moment we are trying to adapt PSR-2, but personally I think this is not quite the right direction. A lot of projects do not fully respect the PSR-(1/2/12) or use its modifications. Other PSR standards may also appear in the future, and they may not be compatible with old PSR. My idea is:

1. The starting configuration does not contain any PSR value, it just makes a beautiful output.
2. We add the option --psr=1, --psr=2, --psr=12 and maybe other value in future.

This will allow projects to migrate easier to the prettier and simplify maintenance. Also use prettier for projects that do not use PSR

/cc @vjeux

Now that we've pretty much completed node type coverage, I'd like to keep moving this forward by starting to test some larger open-source projects to uncover outstanding issues. This might already be possible but at least as of now I'm having trouble wrapping my head around how to do that.
@azz @vjeux @czosel does anyone know if this is possible already with the new plugin api/if not what work needs to be done? I'm guessing we'd ideally want to be able to run via the cli with --debug-check enabled on a directory.

Make sure that when printing code there’s at most one empty line in between lines and none at the beginning or end. We should hopefully be able to reuse the same logic implemented in the JS printer

I think this one can be pretty hard, but Prettier should not remove comment.

-    protected $policies = [
-        // 'App\Model' => 'App\Policies\ModelPolicy',
-    ];
+    protected $policies = [];

input:

if (reallyReallyReallyLongFunction() && $this->reallyreallyreallyreallyLongMethodName()) {
  return true;
}

if ($this || $that && $theOtherThing && $someReallyReallyReallyLongBooleanVariable) {
  return true;
}

output:

if (
reallyReallyReallyLongFunction() && $this->
    reallyreallyreallyreallyLongMethodName()
) {
    return true;
}

if (
$this || $that && $theOtherThing && $someReallyReallyReallyLongBooleanVariable
) {
    return true;
}

PSR12 specifies:

Expressions in parentheses MAY be split across multiple lines, where each subsequent line is indented at least once. When doing so, the first condition MUST be on the next line. The closing parenthesis and opening brace MUST be placed together on their own line with one space between them. Boolean operators between conditions MUST always be at the beginning or at the end of the line, not a mix of both.