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

Fix unknown entity type with multilingual #497

Open
wants to merge 3 commits into
base: 4.0.x
Choose a base branch
from

Conversation

herbdool
Copy link

@civibot civibot bot added the 4.0.x label Aug 13, 2024
@mlutfy
Copy link
Contributor

mlutfy commented Aug 20, 2024

Based on code review, seems good to me (correct way to handle multi-lingual).

However, I don't have a site to test it on.

// @codingStandardsIgnoreStart
global $dbLocale;
// @codingStandardsIgnoreEnd
$locales = \CRM_Core_I18n::getMultilingual();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@herbdool @jackrabbithanna the changes in this file I don't think should being done

In the Supported Entities I think that makes sense as we need an entity for each locale

However in this function I think we are modifying the views query about to be executed so I would have thought we want to only use the active locale rather than all locales because i think the effect of the change in L671-L673 would mean that the last local in the list would be used rather than the active one.

E.g. if you had a list of enabled locales in CiviCRM like en_US,fr_FR,pt_PT or something and you viewed the French version of the view page (based on the url i.e. having /fr/) I think with the changes made in L671-673 and L684-686 this would mean you would only ever get the Portaguese version and also possibly you would get the completely wrong field name as well because it would seem to me that you might end up with a string like title_pt_PT_fr_FR_en_US which doesn't exist in the db as well

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense to me. I'll remove the changes in this file.

Copy link
Author

Choose a reason for hiding this comment

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

Done

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

Successfully merging this pull request may close these issues.

3 participants