Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: internationalization and slug text domain congruence #69

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

MaferMazu
Copy link
Contributor

@MaferMazu MaferMazu commented Mar 8, 2024

Description

This PR:

  • Removes the phpcs:ignore WordPress.WP.I18n.NonSingularStringLiteralContext
  • Changes the text domain used in the internationalization to be congruent with the text domain in the openedx-commerce.php header.
  • Add a test to check that the text domain is congruent.
  • I also excluded composer-setup.php from the phpcs. That is a local file that does not follow the phpcs WordPress standards.

Testing instructions

  • Check if you can still create an Open edX Enrollment Request.
  • Check the tests.

Additional information

I added woocommerce as part of the rules because we added some text in the woocommerce pages.
This change is part of the work to publish the plugin in the WordPress Plugin Directory.

Internationalization: Don't use variables or defines as text, context or text domain parameters.

Please do not use variable names for the text, context or text domain portion of a gettext function, all of them NEED to be strings, otherwise they could not be read by the translation parser.

For example, this is a bad call: esc_html__( 'Translate me.' , $plugin_slug ); , the fix here would be like this esc_html__( 'Translate me.' , 'plugin-slug' );

This is also wrong: esc_html__( 'Your city is ' . $city . '.' , 'plugin-slug' ); , the fix here would be using a placeholder and change it later using printf , like this:
print(

/* translators: %s: Name of a city */

esc_html__( 'Your city is %s.', 'plugin-slug' ),

esc_html($city)

);

Also have in mind that your text domain has to be the same as your plugin's slug (also known as permalink).

The text domain also needs to be added to the plugin header. WordPress uses it to internationalize your plugin meta-data even when the plugin is disabled.

You can read https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/#text-domains for more information.

Example(s) from your plugin:
openedx-commerce/includes/model/class-openedx-commerce-post-type.php:104 _x('Add New', $this->post_type, 'wp-openedx-commerce');

References

WordPress text domains doc: https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/#text-domains
Example of phpcs.xml file for WordPress Text Domain: https://github.com/WordPress/WordPress-Coding-Standards/blob/develop/phpcs.xml.dist.sample#L105

Checklist for Merge

  • Tested in a remote environment
  • Rebased master/main
  • Squashed commits

@openedx-webhooks
Copy link

openedx-webhooks commented Mar 8, 2024

Thanks for the pull request, @MaferMazu! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 8, 2024
@MaferMazu MaferMazu force-pushed the mfmz/fix-text-domain branch from 257e86b to a478523 Compare March 8, 2024 22:20
@MaferMazu MaferMazu marked this pull request as ready for review March 8, 2024 23:39
Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MaferMazu, this PR looks great.

@felipemontoya
Copy link
Member

@MaferMazu now that I'm a proper CC for this repo, my review will unlock the merge button. I'll let you merge at your convenience

@MaferMazu
Copy link
Contributor Author

I will merge this knowing that the release workflow is going to fail, but expecting merge #71 soon to fix the issue.

@MaferMazu MaferMazu merged commit 8461178 into openedx:main Apr 2, 2024
5 checks passed
@openedx-webhooks
Copy link

@MaferMazu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants