Phpinspectionsea: "Ternary operator could be simplified": Option to not break up function calls

[donquixote]

I have a ternary expression like this, using Drupal's t() function:

return !empty($x) ? t('Yes') : t('No');

The plugin suggests to replace it like this:

return t(!empty($x) ? 'Yes' : 'No');

I agree this code by itself is shorter. But not simpler, really.
Esp, when trying to change this code later, one needs to break up the function call again.

Atm one can only completely enable or disable the "Ternary operator could be simplified".
Maybe special case checkboxes could be added to not break up function calls?

[kalessil]

I'll just drop this pattern, it's really ugly =)

[rentalhost]

Oh!!! I like it. Just disable it.

[donquixote]

I'll just drop this pattern, it's really ugly =)

Do you mean just this one case where multiple function calls are combined into one?
I think the "Ternary operator could be simplified" inspection has more cases, which are probably fine.

Maybe the correct label would be "[ ] Combine identical function calls into one." or "[ ] Move ternary expression into function call, to combine multiple calls."

@rentalhost So you would prefer to keep this behavior, but introduce a checkbox for it? Sorry I was not sure if I understand your comment correctly.

[rentalhost]

Hmm... okay... Let me ask you a question first. In which case you "regreted" to use this simplification feature? It is an exception case or is common?

I think that is more valid this inspection respects multiline ternaries, something like:

// Warn in this case:
$var = someMethod() ? t('a') : t('b');

// Ignore that:
$var = someMethod()
    ? t('a')
    : t('b')

Or then implements an intention like "Expand ternary expression" to return it back to original form. But intention are not the focus of this plugin, so I don't know if it could works.

[donquixote]

My problem is I generally want to fix all inspection warnings in my code, and disable all remaining inspections that I think do not apply to my own programming style. In some cases I will use the "suppress for statement", but this should be the rare exception.

If some "false positive" inspection warnings remain for things that I do not intend to change, then these will distract me each time I inspect my code.

On the other hand, I don't want to totally disable any useful inspection.

[donquixote]

In which case you "regreted" to use this simplification feature? It is an exception case or is common?

All the cases I remember were for Drupal's t() function, it is a repeated use case. I don't know what would be a good heuristic here. Maybe it is because the function name is really short? Or maybe I just don't like unnecessarily complex expressions within a function call argument.

I think that is more valid this inspection respects multiline ternaries

I don't think it would be wise to base this on single line vs multiline. It would be weird if you decide to break it to multiple lines for aesthetic reasons, and suddenly see an inspection warning that was not there before.

[rentalhost]

There some case where do you think valid this simplifcation? Can you show some examples? Maybe we can understand "what is ugly/hard to work and what is not".

[donquixote]

"Ternary operator could be simplified" sounds very generic and can mean all kinds of things.

But if we reduce it to cases with function calls:
We could craft an example where the function call is much more verbose than the ternary expression.

Before:

return $success
  ? $kevin->pants->pocket('left')->getContents()
  : $kevin->pants->pocket('right')->getContents();

After:

return $kevin->pants->pocket($success ? 'left' : 'right')->getContents();

Still, I don't know if this is really an improvement.
At least, I don't think either of these versions should cause an inspection warning.
Of course as an intention without inspection warning, I don't mind.

@kalessil Which other cases does the "Ternary operator could be simplified" cover?

[kalessil]

Oh, sorry for the confusion =) I'll drop this case return $kevin->pants->pocket($success ? 'left' : 'right')->getContents();.

[kalessil]

Done

[Fonata]

Drupal's t function is a really special case: it gets parsed by external libraries that don't run the PHP code, but only statically look at it. They don't understand this:

php t($condition ? 'some translatable text' : 'some other translatable text')

As a result, the 'some translatable text' would not be offered to translators and thus remain English much longer. More precisely, they will be picked up once Drupal itself executes the code, which is often after all translations have finished (at least in our company). In that state, getting the translation in is much more effort.

I read both the Java code and this issues code but am currently not sharp enough to discern if $condition ? t('a') : t('b') will trigger the inspection.

[kalessil]

@Fonata : $condition ? t('a') : t('b') will not be reported, as you might noticed already. Can you confirm?

[Fonata]

:+1: Confirmed, it's solved: in EA v2.3.15.1 the inspection is not triggered anymore for the statement above.

[kalessil]

Perfect, thank you for checking 👍