WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 weeks ago

#21616 reviewing defect (bug)

add-textdomain doesn't conform to WP coding standards

Reported by: groovecoder Owned by: ocean90
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 3 years ago.
21616-2.patch (814 bytes) - added by groovecoder 3 years ago.
21616.diff (3.3 KB) - added by GaryJ 16 months ago.
21616.2.diff (5.0 KB) - added by GaryJ 16 months ago.
21616.3.diff (5.0 KB) - added by GaryJ 16 months ago.

Download all attachments as: .zip

Change History (25)

@groovecoder3 years ago

comment:1 @scribu3 years 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 @dd323 years 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 @scribu3 years ago

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

_e('Foo');

will now become:

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

comment:4 @groovecoder3 years 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 @groovecoder3 years 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 @scribu3 years 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 @groovecoder3 years ago

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

comment:8 @scribu3 years 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 @groovecoder3 years ago

Is there a way to tag users in tickets here?

comment:10 follow-up: @scribu3 years 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 @nacin3 years 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.

@groovecoder3 years ago

comment:12 @groovecoder3 years 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 3 years ago by groovecoder (previous) (diff)

comment:13 @groovecoder3 years ago

So are we good with the first patch?

comment:14 @ircbot16 months ago

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

@GaryJ16 months ago

@GaryJ16 months ago

comment:15 @GaryJ16 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.

@GaryJ16 months ago

comment:16 @GaryJ16 months ago

Patch updated to fix incorrect arg when calling process_tokens()

comment:17 @slackbot7 months ago

This ticket was mentioned in Slack in #core by trishasalas. View the logs.

comment:18 @nacin7 months ago

I'm fine with this patch if @ocean90 is.

comment:19 @ocean906 months ago

  • Owner set to ocean90
  • Status changed from reopened to reviewing

comment:20 @groovecoder2 weeks ago

@ocean90 - any movement on this?

Note: See TracTickets for help on using tickets.