search

Automattic / crowdsignal-forms

Gutenberg blocks for Crowdsignal
GNU General Public License v2.0
13 stars 8 forks source link

i18n: Fix omitted dollar signs in printf placeholders #283

Closed jornp closed 1 month ago

jornp commented 1 month ago

This is part of an effort to fix placeholder issues in translatable strings across Automattic products (pxLjZ-8oQ-p2).

%1s is a string placeholder with a minimum width of 1, whereas a numbered placeholder (%1$s) was likely intended. It doesn't cause problems here because there's only one placeholder. However, if there had been more (e.g. %1s and %2s), translators wouldn't be able to change the order of the placeholders, so it's best to keep it safe and fix them here too.

Testing

  1. Start the plugin locally (https://github.com/Automattic/crowdsignal-forms/tree/master/docker#setup).
  2. Activate/connect the plugin.
  3. Go to the Crowdsignal settings (http://localhost:8000/wp-admin/options-general.php?page=crowdsignal-settings) and verify that the texts are still displayed normally and that the links still work:

image

jgcaruso commented 1 month ago

I'm merging this PR and I'll put a card in our team board to release a new build.

@jornp do you think there is urgency to get this released since nothing is actually broken? Or is it just a matter of having a valid repo on github?

jornp commented 1 month ago

@jgcaruso Thanks for the quick review and merging this! This is not urgent at all, so there is no need to create a release specifically for this. 🙂

Or is it just a matter of having a valid repo on github?

Yeah, this exactly! These strings were working fine without the fix too, they just popped up as a warning in my script.

Including it in the next regular release is totally fine!