WordPress.org

Make WordPress Core

Opened 23 months ago

Last modified 2 months ago

#21616 reopened defect (bug)

add-textdomain doesn't conform to WP coding standards

Reported by: groovecoder Owned by:
Milestone: WordPress.org Priority: normal
Severity: normal Version:
Component: I18N Keywords: has-patch dev-feedback
Focuses: Cc:

Description

I used add-textdomain.php from http://i18n.svn.wordpress.org/tools/trunk on my plugin code, but it does not include a space before the closing parenthesis of the gettext function call it alters, which violates http://codex.wordpress.org/WordPress_Coding_Standards#Space_Usage

Attachments (5)

21616.patch (477 bytes) - added by groovecoder 23 months ago.
21616-2.patch (814 bytes) - added by groovecoder 23 months ago.
21616.diff (3.3 KB) - added by GaryJ 2 months ago.
21616.2.diff (5.0 KB) - added by GaryJ 2 months ago.
21616.3.diff (5.0 KB) - added by GaryJ 2 months ago.

Download all attachments as: .zip

Change History (21)

groovecoder23 months ago

comment:1 scribu23 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Hey groovecoder; it's nice that you want to contribute, but:

1) The add-textdomain.php file is not in the WP Core repository, so posting a patch for it on this trac is not appropriate.

2) Patches that only fix coding standards violations are frowned upon, since they cause code churn, without much to show for it.

comment:2 dd3223 months ago

  • Milestone set to WordPress.org
  • Resolution invalid deleted
  • Status changed from closed to reopened

1) The add-textdomain.php file is not in the WP Core repository, so posting a patch for it on this trac is not appropriate.

As the i18n trac has never been used, I don't see anything wrong with having that on the core trac.

2) Patches that only fix coding standards violations are frowned upon, since they cause code churn, without much to show for it.

The changes proposed is in a script which alters existing files, the result of the script does not match the WordPress.org coding standards, as such, It's not just adding a missing space to an existing script.

comment:3 scribu23 months ago

I see. Note that it only partially fixes the problem:

_e('Foo');

will now become:

_e('Foo', 'some-textdomain' );
Last edited 23 months ago by scribu (previous) (diff)

comment:4 groovecoder23 months ago

Right, this script won't fix existing code standard violations, nor should it, IMO. It simply won't introduce new violations into the script.

comment:5 groovecoder23 months ago

In fact now that I've looked I don't think this script can fix the opening parenthesis without a space, since token_get_all only knows about the '(' character.

Unless we start keeping more state during the token loop like:

if ($prev_token == '(' && $token !== ' ') {
    $token = ' ' . $token;
}
$prev_token = $token

Right before the $this->process_token($token, $inplace) line. But that seems like polluting the script with irrelevant functionality.

comment:6 scribu23 months ago

I agree. There's a separate script for tidying up PHP code according to WP standards:

https://github.com/scribu/wp-phptidy

comment:7 groovecoder23 months ago

Cool, what's the next step here? (I'm too used to github pull requests. ;)

comment:8 scribu23 months ago

The next step is waiting for someone to commit it to the i18n repo. I have no idea who has commit access there, besides nacin.

comment:9 groovecoder23 months ago

Is there a way to tag users in tickets here?

comment:10 follow-up: scribu23 months ago

You can assign a ticket to someone, but nacin reads every single trac comment, so he'll see this eventually.

comment:11 in reply to: ↑ 10 nacin23 months ago

Replying to scribu:

You can assign a ticket to someone, but nacin reads every single trac comment, so he'll see this eventually.

By eventually, he means 10 minutes. :-)

I like the idea of keeping more state to make sure we match the coding standard in the document, from comment 5.

groovecoder23 months ago

comment:12 groovecoder23 months ago

I like the idea, and I tried it, but it has too many knock-on effects that cause new violations on otherwise well-formatted code like empty array declarations:

$urls = array( );

and multi-line function calls:

$body = wp_remote_retrieve_body(
    wp_remote_get(

We could add conditionals, but that starts to add too much noise to the code. This script's purpose is just to add text domains - but it shouldn't introduce new coding violations so that a dev who took their time to clean the code before adding text domains doesn't have more work to do.

Dev's could and should use scribu's script separately to clean code.

Last edited 23 months ago by groovecoder (previous) (diff)

comment:13 groovecoder23 months ago

So are we good with the first patch?

comment:14 ircbot2 months ago

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.

GaryJ2 months ago

GaryJ2 months ago

comment:15 GaryJ2 months ago

  • Keywords has-patch dev-feedback added

21616.2.diff is a first pass at this, and is open to suggestions on all aspects.

To check my change was working correctly, I needed some tests. To be able to test sensibly, I neede to be able to just process a string of PHP code, and not individual files, so I needed some refactoring. If this means this diff should go on to a new ticket, which can this fix this ticket, let me know.

The general approach was to make use of the native counter ($index) when looping through the array of tokens, so we can check if the previous token before the closing ) was an instance of whitespace (using coding standards). At the moment, I add a single space to the current token, and strip whatever whitespace was on the end of the previously processed tokens. There's a case where the previous whitespace might have been a line break, not a single space, so this bit could be improved.

With the check of the previous token possible, it becomes a matter of string manipulation to get the desired output.

One thing I did need to change, was to hold on echoing until the file had been completely processed (so that I could remove a previous token during the processing) instead of echo-ing one token at a time. I'm not sure if this will cause any BC behaviour changes.

The new functions are documented, but given a wider ticket scope, I'd refactor process_tokens() even further (and look at making it a protected method).

The tests are simple, but all currently pass.

GaryJ2 months ago

comment:16 GaryJ2 months ago

Patch updated to fix incorrect arg when calling process_tokens()

Note: See TracTickets for help on using tickets.