Skip to content

Conversation

Emkas
Copy link
Contributor

@Emkas Emkas commented Apr 27, 2022

I propose a few changes in messages*.properties which will make the files more 'unite' and consistent.

Here is the list:

  • Add missing insufficientAuthentication property in messages_*.properties
  • Add missing untranslated properties in messages_lt
  • Fixed bad property name in messages_it
  • Remove unnecessary dots from messages_cs_CZ
  • Remove trailing space from messages_ru
  • Add missing badLdapConnection property in messages_*.properties
  • Remove empty messages_nl.properties file

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 27, 2022
@eleftherias eleftherias self-assigned this Apr 27, 2022
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Emkas!

I don't think there is value in adding the placeholders for untranslated properties, such as #ExceptionTranslationFilter.insufficientAuthentication=Full authentication is required to access this resource.
I'm also hesitant to remove the message_nl.properties file in case it causes unexpected behaviour in an application that is expecting it.

The remaining changes look good!
If you could update the PR with the above recommendations I will go ahead and merge it.

@eleftherias eleftherias added in: core An issue in spring-security-core type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 27, 2022
@Emkas
Copy link
Contributor Author

Emkas commented Apr 28, 2022

@eleftherias, thank you for your feedback!
You're the boss, buuuuuut:

  1. I can't imagine how removing empty properties file could break anything. Some one must read this not as a ResourceBundle but as file. Even then, the only thing someone can do with it is a check for existence.
  2. Placeholders are in most of properties right now. For me this is good, because I can translate everything without specialty tool and without jumping through other files (of course if I don't need any context). As for now there are lacks in both messages.properties and messages_en.properties so I had to check both if missing parts are used in code. I was looking for some consistency about this, but I couldn't find any so my guess was that this kind of placeholders are being used in Spring Security.

I mostly did as you asked, but there not much left, as I removed 3 biggest changes :-) Still in two changes I left some placeholders (ExceptionTranslationFilter.insufficientAuthentication). Let me know what you think.

@eleftherias
Copy link
Contributor

Thanks for the quick response @Emkas!

I read through your justification for the 2 additional changes and I agree with you.
Feel free to add those commits back.
Thanks for bringing up these points and for your contribution!

@Emkas
Copy link
Contributor Author

Emkas commented Apr 29, 2022

I'm very pleased to hear that! 2 commits are back in place. As for now, everything is rebased.

Finally only removed part is removing of empty _nl.properties.

@eleftherias eleftherias added this to the 6.0.0-M4 milestone Apr 29, 2022
@eleftherias eleftherias merged commit 33ee305 into spring-projects:main Apr 29, 2022
@eleftherias
Copy link
Contributor

Thanks @Emkas! This is now merged into main.

@Emkas
Copy link
Contributor Author

Emkas commented Apr 29, 2022

Great :)

@Emkas Emkas deleted the properties-clean-up branch April 29, 2022 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in: core An issue in spring-security-core type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants