[BinaryOperatorSpacesFixer] Fix align of = inside calls of methods

[VincentLanglet]

Hi. I found a bug when using align/align_with_space_minimal for =.

Before, any time a ( was found, the check was skipped until the ).

Now, I do this only for ( for function definition and for/foreach/if/while/...

So

$this->foo(function () {
$a = 1;
$aa = 2;
})

aligns the =.

It fix my personal issue (I added a test), and I think it will fix the issue #5667

[coveralls]

Coverage increased (+0.002%) to 93.153% when pulling f7750fd on VincentLanglet:fixEqual into 37e2cbd on FriendsOfPHP:master.

[VincentLanglet]

@SpacePossum @keradus Hi, would you have time to take a look to this fix ? Does something need to be improved ?
Thanks :)

[VincentLanglet]

Friendly ping @keradus @SpacePossum. Is something wrong/missing with my PR ?

[VincentLanglet]

Friendly ping @keradus @SpacePossum ; would you mind taking a look at this bugfix ? :)

[SpacePossum]

Hi and thanks for the PR!

This rule is kind of notorious complex and a good candidate for complete rewrite some day. As such, reviewing is also complex.

The first change seems to aim to never align what is inside the ( ) of a function signature. This seems good to me, I think we can extend with also fn, for example $fn1 = fn($x = 1) => $x + 3;. However the need for skip is because of the second change;

The second change I'm not 100% I follow, why only skip alignment in those new cases and not always in any ( ?

Can you update the sample to the more simple one:

            [
                '<?php
m(
    function ()
    {
        $d["a"]   = 1;
        $d["abc"] = 2;
    }
);
',
                '<?php
m(
    function ()
    {
        $d["a"] = 1;
        $d["abc"] = 2;
    }
);
',
            ],

we also need a test with $fn1 = fn($x = 1) => $x + 3;

I need some more time to check some cases

[VincentLanglet]

Can you update the sample to the more simple one:

Sure, done.

we also need a test with $fn1 = fn($x = 1) => $x + 3;

I don't understand what should be tested.

[SpacePossum]

Thanks for updating/simplify the sample, much appreciated 👍

I'm thinking along these lines,
lets say we have

<?php

fn ($x = 1) => $x + 3;
$f = 123;

it will get fixed to:

--- test.php
+++ test.php
@@ -13,4 +13,4 @@
  fn ($x = 1) => $x + 3;
-$f = 123;
+$f     = 123;

which I think is not correct

The change to if ($token->isGivenKind(T_FUNCTION)) { should be extended not to just test for function signatures but also for these cases, i.e. if ($token->isGivenKind([T_FUNCTION, T_FN])) {

[VincentLanglet]

I'm thinking along these lines, lets say we have

<?php

fn ($x = 1) => $x + 3;
$f = 123;

it will get fixed to:

--- test.php
+++ test.php
@@ -13,4 +13,4 @@
  fn ($x = 1) => $x + 3;
-$f = 123;
+$f     = 123;

which I think is not correct

The change to if ($token->isGivenKind(T_FUNCTION)) { should be extended not to just test for function signatures but also for these cases, i.e. if ($token->isGivenKind([T_FUNCTION, T_FN])) {

You were right, it was fixing. I added the test and the fix.

[SpacePossum]

Thank you @VincentLanglet , apologies for the long wait, PR is much appreciated 👍

[VincentLanglet]

Thank you @VincentLanglet , apologies for the long wait, PR is much appreciated 👍

No problem, I know it's not always easy to find time to maintain such a big project.
I'll be happy to provide some others PR ;)

[stayallive]

This seems to "break" (in my projects) on 2 fronts:

         if ($tenant !== null) {
-            $tenantUrl = $tenant->url;
+            $tenantUrl                     = $tenant->url;
         } elseif (!empty($sessionTenantUrl = session('tenant.url'))) {
-            $tenantUrl = $sessionTenantUrl;
+            $tenantUrl                     = $sessionTenantUrl;
         }

The above seems to be fixed by #6333, the other case is:

-    $parser = new QueryParser(
+    $parser    = new QueryParser(
         $query = 'modifier:single'
     );

So I think this was a good change but missed some edge cases it looks like.

Happy to help where I can but thought to report it here first 👍

[VincentLanglet]

@stayallive #6333 and #6334 are doing some fixes.

If the second PR is still not fixing your use case, please report it