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

Correct password length message grammar #1456

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

rjocoleman
Copy link
Contributor

@rjocoleman rjocoleman commented Feb 9, 2021

Description (*)

This message has bad grammar, it's only in Adminhtml. Not a big issue but it's an easy fix.

Manual testing scenarios (*)

  1. Admin -> Create new customer, admin user, api user etc.

Questions or comments

This will break existing translations - if this is a concern I can revert and change just the en_US translation replacement if that's preferred? This PR updates the en_US translation only.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Admin Relates to Mage_Admin Template : admin Relates to admin template Template : install Relates to install template translations Relates to app/locale labels Feb 9, 2021
Copy link
Contributor

@sreichel sreichel left a comment

Choose a reason for hiding this comment

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

This would break all language packs. I'd just change the translation csv.

@github-actions github-actions bot removed Component: Admin Relates to Mage_Admin Template : install Relates to install template Template : admin Relates to admin template Component: Adminhtml Relates to Mage_Adminhtml labels Feb 21, 2021
@rjocoleman
Copy link
Contributor Author

Thanks @sreichel this has been updated

@kkrieger85 kkrieger85 merged commit 545c20c into OpenMage:1.9.4.x Mar 25, 2021
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
2 runs  ±0  2 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 545c20c. ± Comparison against base commit c7d316a.

@addison74
Copy link
Contributor

As I see this was merged but I suggest changing the PHP code where it is used this phrase

"Password must be at least of %d characters."

to this

Password must be at least %d characters.

@rjocoleman
Copy link
Contributor Author

rjocoleman commented Mar 25, 2021

@addison74 that was the original change in this PR and "the correct fix" but it would break all of the translations/language packs that already exist and rely on that string.

So it's a backward incompatible change that doesn't fit the requirements of (at least) the 1.9.4.x release stream.
Changing the translation only fixes the issue without breaking other translations.

@rjocoleman rjocoleman deleted the password-message branch March 25, 2021 19:37
@addison74
Copy link
Contributor

Checking Magento code for the phrase "Password must be at least of %d characters." I found only two files:

/app/locale/en_US/Mage_Adminhtml.csv:
"Password must be at least of %d characters.","Password must be at least of %d characters."
/app/code/core/Mage/Admin/Model/User.php:
$errors[] = Mage::helper('adminhtml')->__('Password must be at least of %d characters.', self::MIN_PASSWORD_LENGTH);

I understood the change is for a better English and not breaking up other translations but it should be operated in both files to keep consistency. I never saw in a translation different values like this one:

"Password must be at least of %d characters.",""Password must be at least %d characters."

Those who are using translations for Backend should update their translations file. Just switching from Magento vanilla I had to add a lot of translations in different languages.

@sreichel
Copy link
Contributor

Those who are using translations for Backend should update their translations file.

You had to update your german translation for a change that did not even affect you. Does that make sense?

@addison74
Copy link
Contributor

@sreichel: OpenMage ships with one translation US English. If we do a change in the code it should be in all places. I understood your point of view for compatibility with other languages already in use but I do not agree it.

@sreichel
Copy link
Contributor

Then, there is no need for en_US translations at all?

@Flyingmana
Copy link
Contributor

@addison74 you have to see the texts inside the template just as "Keys", not actual texts. To use actual understandable texts is just a decision with certain up- and down- sides. One of them is to easier understand templates.
A grammar error in the template does not hurt for this. If it would be a logical error, that would be different story.

@addison74
Copy link
Contributor

Thank you for your replies. I will respect my opinion related to this commit. You know better what to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
translations Relates to app/locale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants