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

Resolve dev/core#931 by not doing translation on the query if field e… #14187

Merged
merged 1 commit into from
May 2, 2019

Conversation

seamuslee001
Copy link
Contributor

…xists during the upgrade process

Overview

TItle says it all

Before

Adding title field on payment processor fails

After

works

ping @MegaphoneJon @eileenmcnaughton @totten

@civibot civibot bot added the 5.13 label May 2, 2019
@civibot
Copy link

civibot bot commented May 2, 2019

(Standard links)

@seamuslee001
Copy link
Contributor Author

adding merge on pass based on the comments on the 5.14 version

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@seamuslee001 seamuslee001 merged commit 9633590 into civicrm:5.13 May 2, 2019
@seamuslee001 seamuslee001 deleted the dev_core_931 branch May 2, 2019 22:07
@@ -156,12 +156,12 @@ public static function addColumn($ctx, $table, $column, $properties, $localizabl
$domain = new CRM_Core_DAO_Domain();
$domain->find(TRUE);
$queries = [];
if (!CRM_Core_BAO_SchemaHandler::checkIfFieldExists($table, $column)) {
if (!CRM_Core_BAO_SchemaHandler::checkIfFieldExists($table, $column, FALSE)) {
Copy link
Member

Choose a reason for hiding this comment

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

Use-cases:

# Lang config? Column already exists? Column is localizable?
1 Single Lang AlreadyExists Localizable
2 Single Lang AlreadyExists Normal
3 Single Lang NewField Localizable
4 Single Lang NewField Normal
5 Multi Lang AlreadyExists Localizable
6 Multi Lang AlreadyExists Normal
7 Multi Lang NewField Localizable
8 Multi Lang NewField Normal

It reads as if L159 is supposed to determine if the column already exists. I'm not sure this check is right. Suppose:

  • We were running addColumn() with a localized column likecivicrm_event.confirm_email_text.
  • The current config is MultiLang (en_US+fr_FR) and the column AlreadyExists

The actual columns in the DB would be named confirm_email_text_en_US and confirm_email_text_fr_FR. L159 would test for civicrm_event.confirm_email_text, but that would never exist in the table when using multilingual schema? So if you upgrade a multilingual site that already has the column, won't it try to re-add the column and produce a failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@totten the column name is the non localised version of the column name, if we are on a localised database this will then be shown by the fact that it will be halted at the check L164

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so we test for the non localized version first, if its not there, then if it is a database that is to be localized -> check for the localized version if the field is to be localized

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah. 👍

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

Successfully merging this pull request may close these issues.

2 participants