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

PassVsprintfValidator: Add separate regex for arguments only #16359

Closed
wants to merge 1 commit into from

Conversation

jnvsor
Copy link
Contributor

@jnvsor jnvsor commented Nov 11, 2019

Questions Answers
Branch? develop
Description? Fixes a bug preventing the saving of translations containing a %% literal
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #16421
How to test? Attempt a translation containing a literal %% - it will not be saved (Despite a green popup)

This change is Reviewable

Currently it is impossible to submit a translation with a
vsprintf valid %% token because countArgumentsOfTranslation
assumes it shouldn't be a literal '%' but an argument.

A similar issue occurs in CheckTranslationDuplicatesCommand
where it will not recognize the difference between '~' and '%%'
@jnvsor jnvsor requested a review from a team as a code owner November 11, 2019 17:54
@prestonBot
Copy link
Collaborator

Hello @jnvsor!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@prestonBot prestonBot added develop Branch Bug Type: Bug labels Nov 11, 2019
@Progi1984
Copy link
Member

@jnvsor Thank you for your contribution. Could you open an issue for describing the bug ? Thanks

@jnvsor
Copy link
Contributor Author

jnvsor commented Nov 15, 2019

@Progi1984 Added a bug report

Copy link
Contributor

@PierreRambaud PierreRambaud left a comment

Choose a reason for hiding this comment

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

Hi and sorry for the delay,
why not updating the ̀regexSprintfParams` property?

@jnvsor
Copy link
Contributor Author

jnvsor commented Jan 16, 2020

Because you still want isSprintfString to return true if the only argument is a %%, otherwise you might break existing strings

@PierreRambaud
Copy link
Contributor

@jnvsor This could be done with a single regex 🤔

@jnvsor
Copy link
Contributor Author

jnvsor commented Jan 18, 2020

This could be done with a single regex

By using capture groups? I presume there was a performance reason not to, given that someone went to the effort of making the groups in this regex non-capturing in the first place.

The options as I see them are:

  1. Have one regex that matches %% and ignore bug Cannot add literal % to translation strings #16421
  2. Have one regex that doesn't match %% and break existing translation strings with %% in them (Though given the existence of Cannot add literal % to translation strings #16421 there may not be many of these)
  3. Have two regexes
  4. Use capture groups to distinguish between matches and argument counts (Would need more testing if we went that way)

@PierreRambaud
Copy link
Contributor

@jnvsor We should be able to use each characters we want 🤔
Having two regexes = two to maintain.
let's continue the discussion in the issue :)

@matks
Copy link
Contributor

matks commented Sep 15, 2020

@PierreRambaud Any update :) ?

@PierreRambaud
Copy link
Contributor

@PierreRambaud Any update :) ?

Waiting for a feedback, this solution is not clean IMHO

@matks
Copy link
Contributor

matks commented Aug 11, 2021

Hello, it seems this PR has no activity since 2020.

I close this PR, following our review guidelines "Pull Requests may be closed after 30 days of inactivity".

If however you are still interested in it, please ping us 😉 we can reopen.

Thank you for your contribution.

@matks matks closed this Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Type: Bug develop Branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot add literal % to translation strings
5 participants