Question: "unused use statements" and php-cs-fixer

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Question: "unused use statements" and php-cs-fixer

weierophinney
Administrator
Hey, all --

We're getting a ton of php-cs-fixer errors currently due to "unused
use statements". Unfortunately, the situation is not straight-forward.

php-cs-fixer is detecting when a "use" statement is in place, but the
*code* does not exercise it. Unfortunately, we currently have a rule
in place that allows adding "use" statements for classes referred to
only in docblocks, in order to keep them more readable.

I see a few paths forward:

- Alter our php-cs-fixer configuration to not scan for unused use
statements. This has the downside that if unused use statements *are*
present, they will not get detected. On the flip side, all tools work
correctly going forwards.
- Run the tool once, make no further changes. This will break tools
such as phpdocumentor, IDEs, etc., as typehints derived from
annotations will no longer resolve properly.
- Run the tool, but check each class for class names used in
annotations only, and fix them. This will be relatively tedious, but
ensure no more false positives from php-cs-fixer, and proper
resolution in other tools.
- See if we can get a new flag in place in php-cs-fixer to check use
statements also against annotations.

The problem right now is that *all* builds are failing, due to CS
errors based on unused use statements. As such, all pull requests have
to be checked manually for errors.

Thoughts on how to proceed?

--
Matthew Weier O'Phinney
Project Lead            | [hidden email]
Zend Framework          | http://framework.zend.com/
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc
Reply | Threaded
Open this post in threaded view
|

Re: Question: "unused use statements" and php-cs-fixer

Marco Pivetta


On 3 March 2014 21:38, Matthew Weier O'Phinney <[hidden email]> wrote:
Hey, all --

We're getting a ton of php-cs-fixer errors currently due to "unused
use statements". Unfortunately, the situation is not straight-forward.

php-cs-fixer is detecting when a "use" statement is in place, but the
*code* does not exercise it. Unfortunately, we currently have a rule
in place that allows adding "use" statements for classes referred to
only in docblocks, in order to keep them more readable.

I see a few paths forward:

- Alter our php-cs-fixer configuration to not scan for unused use
statements. This has the downside that if unused use statements *are*
present, they will not get detected. On the flip side, all tools work
correctly going forwards. 
- Run the tool once, make no further changes. This will break tools
such as phpdocumentor, IDEs, etc., as typehints derived from
annotations will no longer resolve properly.
- Run the tool, but check each class for class names used in
annotations only, and fix them. This will be relatively tedious, but
ensure no more false positives from php-cs-fixer, and proper
resolution in other tools.
- See if we can get a new flag in place in php-cs-fixer to check use
statements also against annotations.

The problem right now is that *all* builds are failing, due to CS
errors based on unused use statements. As such, all pull requests have
to be checked manually for errors.

Thoughts on how to proceed?

--
Matthew Weier O'Phinney
Project Lead            | [hidden email]
Zend Framework          | http://framework.zend.com/
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc

Is this caused by a cs-fixer update?

I see that there are several commits in PHP-CS-Fixer that are related with unused USE statements: https://github.com/fabpot/PHP-CS-Fixer/commit/e7f770415d587e388d00fc0a8a62e3007ea16dc1 (just one of 'em).

I'd eventually cap it to a specific version of CS-fixer until we've migrated to PHPCS, if possible.

The choice of avoiding PHPCS was done because of the I/O+memory impact it had, but I'd also add that we moved to async test execution meanwhile.
That would allow re-introducing PHPCS (and yes, that will explode in a number of minor CS issues for now).

Marco Pivetta

http://twitter.com/Ocramius     

http://ocramius.github.com/
Reply | Threaded
Open this post in threaded view
|

Re: Question: "unused use statements" and php-cs-fixer

Stefano Torresi
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Question: "unused use statements" and php-cs-fixer

Pádraic Brady
Hi Matthew,

What is the specific coding standard used and what does it actually specify?

I haven't brought it up previously, but once I have finished working
on Composer one of the tasks I have on my list is to do a proper
survey of coding standard compliance in PHP, correlate that to
PHP_CodeSniffer interpretations, and possibly request permission to
poke PHP-FIG about updating their standard (I can do it under my own
name otherwise - it's just something that has been bugging me and Phil
Sturgeon to no end about both phpcs and php-cs-fixer).

For example, PSR-2 is completely silent on how to utilise use
statements other than to specify their formatting. So this appears to
be a simple case of PHP-CS-Fixer being on the wrong side of compliance
checking. Their warning, or whatever, is unnecessary. That makes it
their bug to fix.

Paddy

On 3 March 2014 21:09, Stefano Torresi <[hidden email]> wrote:

> I don't understand the cause of the issue. That commit is two months old,
> isn't php-cs-fixer self updating on each build? Besides, I follow the same
> rule as ZF2 and don't have this issue! I just tried to re-start a travis
> build that should be affected, but it passed. Also I don't see *every* build
> failing. Why is the issue so inconsistent?
> Are we sure this is not just a php-cs-fixer bug? It has happened before that
> some patch introduced some regression and CI builds blew up all over the php
> world -.-
>
> Stefano Torresi
> Web Developer
>
>
> 2014-03-03 21:57 GMT+01:00 Marco Pivetta <[hidden email]>:
>
>>
>>
>> On 3 March 2014 21:38, Matthew Weier O'Phinney <[hidden email]> wrote:
>>>
>>> Hey, all --
>>>
>>> We're getting a ton of php-cs-fixer errors currently due to "unused
>>> use statements". Unfortunately, the situation is not straight-forward.
>>>
>>> php-cs-fixer is detecting when a "use" statement is in place, but the
>>> *code* does not exercise it. Unfortunately, we currently have a rule
>>> in place that allows adding "use" statements for classes referred to
>>> only in docblocks, in order to keep them more readable.
>>>
>>> I see a few paths forward:
>>>
>>> - Alter our php-cs-fixer configuration to not scan for unused use
>>> statements. This has the downside that if unused use statements *are*
>>> present, they will not get detected. On the flip side, all tools work
>>> correctly going forwards.
>>>
>>> - Run the tool once, make no further changes. This will break tools
>>> such as phpdocumentor, IDEs, etc., as typehints derived from
>>> annotations will no longer resolve properly.
>>> - Run the tool, but check each class for class names used in
>>> annotations only, and fix them. This will be relatively tedious, but
>>> ensure no more false positives from php-cs-fixer, and proper
>>> resolution in other tools.
>>> - See if we can get a new flag in place in php-cs-fixer to check use
>>> statements also against annotations.
>>>
>>> The problem right now is that *all* builds are failing, due to CS
>>> errors based on unused use statements. As such, all pull requests have
>>> to be checked manually for errors.
>>>
>>> Thoughts on how to proceed?
>>>
>>> --
>>> Matthew Weier O'Phinney
>>> Project Lead            | [hidden email]
>>> Zend Framework          | http://framework.zend.com/
>>> PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc
>>
>>
>> Is this caused by a cs-fixer update?
>>
>> I see that there are several commits in PHP-CS-Fixer that are related with
>> unused USE statements:
>> https://github.com/fabpot/PHP-CS-Fixer/commit/e7f770415d587e388d00fc0a8a62e3007ea16dc1
>> (just one of 'em).
>>
>> I'd eventually cap it to a specific version of CS-fixer until we've
>> migrated to PHPCS, if possible.
>>
>> The choice of avoiding PHPCS was done because of the I/O+memory impact it
>> had, but I'd also add that we moved to async test execution meanwhile.
>> That would allow re-introducing PHPCS (and yes, that will explode in a
>> number of minor CS issues for now).
>>
>> Marco Pivetta
>>
>> http://twitter.com/Ocramius
>>
>> http://ocramius.github.com/
>
>



--

--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com
Zend Framework Community Review Team
Zend Framework PHP-FIG Representative
Reply | Threaded
Open this post in threaded view
|

Re: Question: "unused use statements" and php-cs-fixer

Ben Scholzen 'DASPRiD'
In reply to this post by weierophinney
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Question: "unused use statements" and php-cs-fixer

Pádraic Brady
Hi Ben,

PHPCS is pretty strict and not necessarily wholly compliant - and it
is a true checker rather than a limited CS fixer. We'd have a
significant legacy problem on a number of PSR-2's recommendations. Not
saying it's insurmountable, but it would be quite the time investment
for someone...else ;).

Paddy

On 4 March 2014 04:18, Ben Scholzen 'DASPRiD' <[hidden email]> wrote:

> I have no clue whether I asked this already, but… why didn't we switch
> to PHPCS yet for coding-standard checks within Travis? Their PSR-2 suite
> is complete by now as far as I remember. In any case much more complete
> than PHP-CS-Fixer.
>
> On 03.03.2014 21:38, Matthew Weier O'Phinney wrote:
>> Hey, all --
>>
>> We're getting a ton of php-cs-fixer errors currently due to "unused
>> use statements". Unfortunately, the situation is not straight-forward.
>>
>> php-cs-fixer is detecting when a "use" statement is in place, but the
>> *code* does not exercise it. Unfortunately, we currently have a rule
>> in place that allows adding "use" statements for classes referred to
>> only in docblocks, in order to keep them more readable.
>>
>> I see a few paths forward:
>>
>> - Alter our php-cs-fixer configuration to not scan for unused use
>> statements. This has the downside that if unused use statements *are*
>> present, they will not get detected. On the flip side, all tools work
>> correctly going forwards.
>> - Run the tool once, make no further changes. This will break tools
>> such as phpdocumentor, IDEs, etc., as typehints derived from
>> annotations will no longer resolve properly.
>> - Run the tool, but check each class for class names used in
>> annotations only, and fix them. This will be relatively tedious, but
>> ensure no more false positives from php-cs-fixer, and proper
>> resolution in other tools.
>> - See if we can get a new flag in place in php-cs-fixer to check use
>> statements also against annotations.
>>
>> The problem right now is that *all* builds are failing, due to CS
>> errors based on unused use statements. As such, all pull requests have
>> to be checked manually for errors.
>>
>> Thoughts on how to proceed?
>>
>
>
> --
> Ben Scholzen 'DASPRiD'
> Community Review Team Member | [hidden email]
> Zend Framework               | http://www.dasprids.de



--

--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com
Zend Framework Community Review Team
Zend Framework PHP-FIG Representative
Reply | Threaded
Open this post in threaded view
|

Re: Question: "unused use statements" and php-cs-fixer

Ben Scholzen 'DASPRiD'
CONTENTS DELETED
The author has deleted this message.