NoSpacesAfterFunctionNameFixer - Don't touch parens-not-meaningful false-functions

[guilliamxavier]

Fixes #4817 (and #1237)

[SpacePossum]

@julienfalque isn't this a bug if it is targeting 2.15?

[julienfalque]

This could be considered a bugfix as well indeed, I have no preference here. We merge "enhancement" PRs on lowest branches just like bugfixes anyway.

[SpacePossum]

should this list contain T_DECLARE as well, like declare (ticks=1); => declare(ticks=1); ?

[julienfalque]

Actually I would do the opposite and handle it in #4435 instead.

[guilliamxavier]

Quick facts:

• I have always seen declare(strict_types=1); (without space), never declare (strict_types=1);. Also, unlike echo/include, for declare parentheses are meaningful (and required). So it looks like it could be added to this list...
• But the PHP manual documents it under "Control Structures", alongside if and switch (for which parentheses are required too), and PSR-2 says "There MUST be one space after the control structure keyword"...
• However, PSR-12 adds a specific "Declare statements MUST contain no spaces".

So, what about handling that as a separate issue (i.e. not here, but not in #4435 either)?

[SpacePossum]

I don't like all these exceptions, why T_LIST and not T_DECLARE, all arguments made here can done the other way around by someone else next week and than we are back here.

T_LIST is a language construct not a function, why is touched, PSR says nothing about language constructs

So I'm -1 for any language construct here and +1 for a new fixer for these with opt-in for each so everyone can be happy.

[guilliamxavier]

@SpacePossum: Fair point (the problem actually dates back to the initial creation of FunctionCallSpaceFixer in #683), and good idea 🙂 Just 2 questions:

• You would keep T_VARIABLE in this list (i.e. still fix $f (1) to $f(1)), right?
• Do you want me to update this PR accordingly, or would it need a new PR?

Thanks

[julienfalque]

I do think that require (and similar keywords) should be treated like echo but not everyone agrees. That includes @SpacePossum but we can safely assume that more people not involved in the discussion here will disagree too and consider this change a regression.

Actually this reasoning might even apply to echo, maybe better postponing this PR until we agree to a solution in #4859?

[SpacePossum]

Sorry but what's the argument for reverting require/include but not print?

I'm sorry, I try again:

• if someone want to change something here, the author must make the argument for the change, not the reviewer
• up to now the argument that has been made over and over again in all kinds of different wording was; I don't like what the fixer does, I have a different preference. I say preference because I haven't seen any evidence the fixer does not what it should do based on the agreed workings when it was created
• if you want to change how it works, we keep the BC promise and do it on the next minor

I'm gonna say, I don't like even to see print and echo changed anymore. Simplify because I'm done defending the current fixer.

[guilliamxavier]

I agree with your points, except for this part:

I haven't seen any evidence the fixer does not what it should do based on the agreed workings when it was created

I find #683 (comment) and #1237 (comment) (both ignored, regrettably) are evidence of disagreements as soon as it was created...

Anyway, since you closed this PR, I hope to see constructive propositions in #4859

[SpacePossum]

Why are you ignoring me telling you that you can have what you want just on the next minor that will be released at the same time as the next patch release? Can you please respond to the people who try to give you what you want?

[guilliamxavier]

No offense, I said I agree with your [other] points, including "BC promise" and "next minor". I'm simply waiting for #4859 to define exactly what to do (for the next minor, hopefully) 🙂