[MAGNOLIA-5931] AbstractI18nContentSupport.getProperty not returning right default value based on default locale Created: 24/Sep/14  Updated: 06/Oct/14  Resolved: 03/Oct/14

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: None
Fix Version/s: 5.3.4

Type: Bug Priority: Neutral
Reporter: Evzen Fochr Assignee: Mikaël Geljić
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
relation
is related to MGNLUI-3128 FormBuilder: defaultLocale in dialogs... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

Considering a setup where default and fallback locale are configured differently (e.g. default (unsuffixed) locale is German, fallback locale is English):

When current locale is German, AbstractI18nContentSupport#getProperty() first tries to resolve "property_de" (note the suffix)
— because that doesn't exist, it keeps looking up for the next locale, down to the fallback one, that is English.

Whenever current locale is the default locale, it should also try to lookup the property un-suffixed.



 Comments   
Comment by Mikaël Geljić [ 26/Sep/14 ]

Reopening to check potentially incorrect behavior:

  • Context: the patch is in the "while" loop where it tries to get a non-empty property for the current locale, if there is none it gets the next candidate content locale (e.g. pt for pt-br), and so on, ultimately down to the fallback locale.
  • Current fix: if current locale is the default one and no default (unsuffixed) property exists, it goes straight to the fallback locale (else statement), and bypasses the potential intermediate candidate locales.

In any case, to be on the safe side I would recommend to write a basic AbstractI18nContentSupportTest first for this resolution mechanism, then add the failing test corresponding to the ticket, fix it and make sure the patch doesn't break other basic tests.

Comment by Jan Haderka [ 29/Sep/14 ]

Yes, that's what I said in the review - there should be a default value set in the code and to be used when it is not set explicitly by user rather then highjacking the fallback that is meant to be dynamic.

Comment by Mikaël Geljić [ 29/Sep/14 ]

That's more or less what the private field (mis)named fallbackLocale does, isn't it?
Curious to see in particular where you see the fallback locale hijacked in current code base.

In any case cleaning up the resolution approach is probably not relevant to this ticket.
As far as I understood, here is just a matter of looking up the property without suffix for the default locale; the rest of the behavior I would leave untouched.

Edit: I just rephrased the ticket to remove the fallback parts, as we seemed to agree it was irrelevant

Comment by Jan Haderka [ 29/Sep/14 ]

Generated at Mon Feb 12 04:09:44 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.