5

Resolved

CultureRecord.Culture can be set to null (breaking everything)

description

Today our site' auto restart failed because we found that, after analysing the problem, the default culture record (that should contain en-US as 'Culture') had its 'Culture' set to NULL.

I found 2 locations in the Orchard.Localization' AdminController that does this site-breaking behavior:

Inside Translate(int id, string to)
        if (contentItem.As<LocalizationPart>().Culture != null)
            contentItem.As<LocalizationPart>().Culture.Culture = null;
And inside TranslatePOST(int id, Action<ContentItem> conditionallyPublish)
        if (!ModelState.IsValid) {
            Services.TransactionManager.Cancel();
            model.SiteCultures = _cultureManager.ListCultures().Where(s => s != _localizationService.GetContentCulture(contentItem) && s != _cultureManager.GetSiteCulture());
            var culture = contentItem.As<LocalizationPart>().Culture;
            if (culture != null) {
                culture.Culture = null;
            }
            model.Content = _contentManager.BuildEditor(contentItem);
            return View(model);
        }
I see no good reason to set the Culture property of the CultureRecord to NULL in any of both locations.

Could someone look into this and state ASAP what is the correct action to take here? This killed our website today!

I assume that the ModelState wasn't valid for the first time ever when our client was translating an item, that is (again I assume) the reason why this issue never occured for us before!

comments

CSADNT wrote May 22, 2014 at 6:00 PM

This is a very strange code, Orchard takes an existing contentItem (often Page) and do a pre-cloning on it to allow copying its values for a new item, I thing I found a bug in the process here
https://orchard.codeplex.com/workitem/20674

CSADNT wrote May 22, 2014 at 6:01 PM

I think ... but also I am quite a thing behing the keyboard

AimOrchard wrote May 30, 2014 at 9:49 AM

Could someone provide a fix for this issue? This is still breaking our site whenever the issue occurs...

CSADNT wrote May 30, 2014 at 10:18 AM

Have you tried what I suggested concerning the position of cancel ?

AimOrchard wrote May 30, 2014 at 10:33 AM

It should never have to set that value to null... Even if the transaction would be canceled or not.

CSADNT wrote May 30, 2014 at 10:42 AM

I agree but this little modification could solve your pb until some new code is written

AimOrchard wrote Jul 11, 2014 at 10:03 AM

Could someone 'official' please look into this bug...

Or is the potential ability to break sites not critical anymore these days?

CSADNT wrote Jul 11, 2014 at 1:04 PM

If you call Services.TransactionManager.Cancel() where it is missing, there are less problems.
You could try my implementation where I tryed to solve some of the pbs related to localization, especially the fact that some controller parameters are never used in actual code, leading to bad behaviours when creating a new version of content based on another culture. (replacing the whole controller)
https://bitbucket.org/csurieux/datwendo.localization

AimOrchard wrote Jul 11, 2014 at 2:03 PM

I know, but why isn't this getting picked by Orchard.

What is the use of reporting critical bugs, if they aren't picked up at all...

(fyi: not 'hitting' on you there CSADNT, you're doing a great job :P)

AimOrchard wrote Jul 16, 2014 at 2:48 PM

Anyone?

AimOrchard wrote Aug 6, 2014 at 12:46 PM

?

Jetski5822 wrote Sep 3, 2014 at 11:08 PM

@aimorchard can you check the latest Localization branch, this should now be resolved there.

jgreywolf wrote Sep 8, 2014 at 8:19 PM

I was running into this same problem, however I solved it in a different manner.
What I found is that when the Content Item is saved, the following method is called:
        void ILocalizationService.SetContentCulture(IContent content, string culture) {
            var localized = content.As<LocalizationPart>();
            if (localized == null || localized.MasterContentItem == null) {
                return;
            }

            localized.Culture = _cultureManager.GetCultureByName(culture);
        }
What happened is that if the content included the localization part (so localized was != null), but it did not have a MasterContentItem (because it was the first one), the localized.Culture object was never being set. For now I have addressed this by adding an additional check to ensure that the culture gets set even if there is no MasterContentItem
  if (localized == null || (localized.MasterContentItem == null && localized.Culture != null)) {
                return;
            }

As an interesting side point, while I was debugging the issue I also discovered an odd "side effect" that was occurring which I have not yet quite tracked down the source of.
What I noticed is that sometimes the culture record in the database was being updated after the fact.
([Orchard_Localization_LocalizationPartRecord] table in database)
Specifically:
  • Create a new content item (Content A) that includes a ContentPicker/Creator (do not save)
  • Add new content item within the ContentCreator (Content B), and save
  • At this time, Content B will have a Culture of 0 in the database record (Content A will not yet exist)
  • Save Content A.
  • Now Content A will have culture record of 0 in the database as well.
  • Create a NEW content item (Content C)
  • Look at record for Content B in database, Culture has been updated to 2.
  • Add new content item in Content C from ContentCreator.
  • Look at the record for Content A in database - Culture has been updated to 2.
However, this only seemed to happen when you immediately created the new Content Items, and that they had the same Content Type (though I did not do a lot to confirm that last)

Jetski5822 wrote Sep 12, 2014 at 10:24 AM

I have just rewritten the whole Admin Controller for Localization, including part of the controller. I recon I have crushed a lot of the inconsistencies.

Jetski5822 wrote Oct 12, 2014 at 3:42 PM

Just pushed localization to 1.x this should now be resolved.