DX: Reduce usage of PhpCsFixer\DocBlock\Line::__toString

[kubawerlos]

No description provided.

[julienfalque]

Shouldn't the method make a @trigger_error('...', E_USER_DEPRECATION) call?

[SpacePossum]

risky, because a lot of IDE's call this method to show preview data when debugging

[kubawerlos]

Wouldn't @ make it transparent for IDEs?

[keradus]

Please, provide reasoning why to deprecate this in first place.

[kubawerlos]

Anyway, the magic is heavily used - in DocBlock and Annotation classes as well - should we get rid of all the __toString functions and replace:

implode('', $lines);

with:

implode(
    '',
    array_map(
        function (Line $line) {
            return $line->getContent();
        },
        $lines
    )
);

?

[kubawerlos]

Please, provide reasoning why to deprecate this in first place.

@keradus not sure not now, initially was to simply avoid magic method.

[keradus]

1. no reasoning is indicator to not pursuit the idea
2. lack of @trigger_error is an issue, especially, as you noticed yourself, we reduce usage of that magic method, but not get rid of it
3. I removed deprecation part of this PR and now, I will merge it as is - if we want to deprecate sth indeed, we need to have good reasoning for it, deprecation notification in place and stop using deprecated code in our codebase.

☝️

[keradus]

Already applied in #4088

[keradus]

note:
i'm not for using magic methods everywhere, but we may use them sometimes.

If you want to get rid of them, please deprecate them (with real deprecation notice) everywhere, in all PhpCsFixer\DocBlock\* and AllowedValueSubset (the __invoke)

[SpacePossum]

1. no reasoning is indicator to not pursuit the idea

I would like to note that using __toString() comes with some drawbacks which is why I never use it:
1 - finding "usages" is near impossible, because;
2 - it allows for implicit string casting of an object
3 - it is not a descriptive method name
4 - it is invalid to throw exceptions from the method, which makes it more limited than any regular method for no reason

Magic methods like __invoke and __isset have a far better defined role and definition and besides being magic have nothing in common with __toString.

I'm fine with not merging though as it might not be worth adding this BC break now :)

[kubawerlos]

I forget to close it, I've checked and it would not be that easy to remove the magic methods.

[keradus]

not sure not now, initially was to simply avoid magic method.

no reasoning is indicator to not pursuit the idea

just saying that change for a change is not a reason, it has to have background

as said later, i'm not negative for such change, but it has to be applied fully