WPCS in PemFTP library

[tristup]

Hi Team,

I found the following file does not follow the proper WPCS rules. I am proposing the fix.
Patch file attached.

File Location: wp-admin/includes/class-ftp-sockets.php

[sabernhardt]

PemFTP is an external library and does not need to follow WordPress coding standards. WordPress has modified its files in #29628 and #33432, but I do not think it is worth updating the files for WPCS.

[sabernhardt]

[59846] recently committed coding standards changes to class-ftp.php, which is also part of PemFTP.

[SirLouen]

@tristup the patch is wrong. Try to resubmit it again, no necessary, but ideally to Github ​Check this

@sabernhardt I think there is plenty of work to be done PHPCS-wise in this file

FILE: /home/user/src/wordpress/wordpress-develop/src/wp-admin/includes/class-ftp-sockets.php
----------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 816 ERRORS AND 69 WARNINGS AFFECTING 143 LINES

[sabernhardt]

If one of the old PemFTP files follows coding standards in its entirety, I would think all three files should. Then the code sniffer might not need to ​ignore them.

• The ticket description mentions class-ftp-sockets.php.
• The patch proposes changes for class-ftp-pure.php, and it fails to apply to my installation (likely due to incompatible formatting).
• [59846] revised some lines in class-ftp.php, establishing a precedent, but that was incomplete.

Documentation changes could be worth more than meeting coding standards. Several functions are not documented, and some of the comments are in French (including a few abbreviated French words).

[SirLouen]

Replying to sabernhardt:

If one of the old PemFTP files follows coding standards in its entirety, I would think all three files should.

I think that at this point, the code is being maintained by WordPress, not by the original author. It's not like PHPMailer AFAIK. So if WordPress is going to maintain the FTP client, the logical thing is to have the WP standards, so I think that all 3 exclusions should be removed.

I've opened a new ticket to keep things in the right place: #63436
Before starting to work in a patch for this ticket, the exclusions should be confirmed to be removed as @sabernhardt suggested

[sabernhardt]

I do not recommend removing the exclusion (#63436).

PR 8799's ​GitHub Action for coding standards lists 4,383 errors, and many of them relate to naming:

• 3 Constants must be uppercase
• 5 Class name must begin with a capital letter (plus "Class name is not valid" errors for ftp_base, ftp_pure and ftp_sockets)
• 7 Member variable is not in valid snake_case format
• 41 Object property is not in valid snake_case format

[SirLouen]

Replying to sabernhardt:

I do not recommend removing the exclusion (#63436).

PR 8799's ​GitHub Action for coding standards lists 4,383 errors, and many of them relate to naming:

• 3 Constants must be uppercase
• 5 Class name must begin with a capital letter (plus "Class name is not valid" errors for ftp_base, ftp_pure and ftp_sockets)
• 7 Member variable is not in valid snake_case format
• 41 Object property is not in valid snake_case format

This can be fixed easily.

But in that case, does it make sense, to keep the PHPCS update in class-ftp? After all, it’s editing the core code without the original library support.

[sabernhardt]

[59846] was a small improvement, not worth reverting.

[SirLouen]

Replying to sabernhardt:

[59846] was a small improvement, not worth reverting.

In fact I'm reading now that jbaudras actually warned about taking actions on this file... Weird ngl.

[sabernhardt]

The contributor handbook discourages "suggestions to rename long-standing functions or conform to more-recently established coding standards." Rewriting this library would involve both those types of ​code refactoring.

Closing as not planned.

[SirLouen]

@sabernhardt: please keep in mind that, according to one of the latest dev-chat (as I commented in #63436), there is currently over the table, the possibility of auditing external libraries to detect which could need to be integrated into core, and transitionally, conform with CS (and maybe rewriting of some functions if this requires CS compliance). Noone should be using external library functions as they are shall not be public methods, or they should have an equivalent public alternative.

Anyway, I think that this is just one among many, so I would have instead closed as maybelater, not wontfix