backdrop-contrib / i18n

Collection of modules to extend Backdrop CMS multilingual capabilities
https://backdropcms.org/project/i18n
GNU General Public License v2.0
2 stars 5 forks source link

Unknown column locales_source.textgroup when running i18n_update_1003() on a site upgrading from D7 #75

Closed herbdool closed 3 years ago

herbdool commented 3 years ago

I've got a site that we upgraded to Backdrop last year (a D7 site that had i18n previously but wasn't upgraded with it). I'm trying the i18n module and getting:

Column not found: 1054 Unknown column 'textgroup' in 'where clause': SELECT s.lid AS lid FROM {locales_source} s WHERE (textgroup = :db_condition_placeholder_0) ; Array ( [:db_condition_placeholder_0] => blocks ) in i18n_update_1003()

If I recall correctly, core drops textgroup, right? So maybe we need to wrap part of update 1003 in a check that textgroup exists.

I'm trying it with v1.x-1.0.0-alpha2.

I should add that I only enabled i18n and i18n_string.

herbdool commented 3 years ago

Perhaps because textgroup is only being added in the install hook with i18n_string and if a D7 site already had it installed it won't run again.

indigoxela commented 3 years ago

I've got a site that we upgraded to Backdrop last year (a D7 site that had i18n previously but wasn't upgraded with it).

Yes, this might be the culprit here. The schema of the table doesn't match. I'll try to dig into this soon.

indigoxela commented 3 years ago

@herbdool I'm not sure, which approach is the right one here.

Core dropped the column in locale_update_1000(), i18n_string catches that since January this year in an update hook.

But you ran the upgrade without i18n – did I get that right? Or before i18n_string_update_1001() has been implemented (so core dropped the column and the hook came too late)?

How would you solve that?

jenlampton commented 3 years ago

I'm seeing the same thing on a D7 site that's upgrading with i18n.

Failed: PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'textgroup' in 'where clause': SELECT s.`lid` AS `lid` FROM {locales_source} s WHERE (`textgroup` = :db_condition_placeholder_0) ; Array ( [:db_condition_placeholder_0] => blocks ) in i18n_update_1003() (line 178 of backdrop/docroot/modules/contrib/i18n/i18n.install).

The update hook in i18n needs to come before the update hook in core that removed the column. That's done by implemeting hook_update_dependencies().

indigoxela commented 3 years ago

@jenlampton I don't think that we can fix this.

The i18n update hooks run by intention in the order they run, trying to avoid problems with core's destructive action on locale's tables.

But under certain circumstances this may fail. Can you post a list of your installed modules? Possibly there are additional update dependencies that break the order.

jenlampton commented 3 years ago

I have a PR that fixes it. I'm trying to push it up now, but the version of the module I get from drush dl is really different from the 1.x-1.x branch, which seems to be missing i18n_update_dependencies(). I think my remotes are all funky... this may take me a moment to straighten out.

Here's my diff:

diff --git a/i18n.install b/i18n.install
index abbb0a3..e09b598 100644
--- a/i18n.install
+++ b/i18n.install
@@ -53,6 +53,10 @@ function i18n_update_dependencies() {
   $dependencies['i18n'][1003] = array(
     'block' => 1004,
   );
+  // Make these changes before the `textgroup` column is dropped.
+  $dependencies['locale'][1000] = array(
+    'i18n' => 1003,
+  );
   return $dependencies;
 }

The i18n update hooks run by intention in the order they run, trying to avoid problems with core's destructive action on locale's tables.

Yes, that's exactly what I was hoping to use hook_update_dependencies() for here too :)

But under certain circumstances this may fail. Can you post a list of your installed modules? Possibly there are additional update dependencies that break the order.

Here are the list of the modules:

addressfield
ajax_markup
better_exposed_filters
bueditor
cacheexclude
cloudflare
colorbox
diff
disqus
easy_social
entity_plus
field_group
google_tag
i18n
imagemagick
markdown
markdowneditor
metatag
multiple_selects
nodequeue
reference
slick
typogrify
video_embed_field
views_autocomplete_filters
views_limit_grouping
views_load_more
xmlsitemap
indigoxela commented 3 years ago

I have a PR that fixes it. I'm trying to push it up now, but the version of the module I get from drush dl is really different from the 1.x-1.x branch, which seems to be missing i18n_update_dependencies().

Can you please check that your fork of i18n is up to date?

Your code seems to duplicate a dependency already declared here: https://github.com/backdrop-contrib/i18n/blob/1.x-1.x/i18n_string/i18n_string.install#L245

jenlampton commented 3 years ago

Can you please check that your fork of i18n is up to date?

My fork was fine, but my local copy had issues. A new checkout worked :)

Your code seems to duplicate a dependency already declared here: 1.x-1.x/i18n_string/i18n_string.install#L245

Aha! I think you might have found the answer :) My site is not using the i18n_strings submodule, so those hooks won't be called on my site. I pushed a PR that moves the dependency to the i18n module which allows the updates to complete for my site. I'm not sure what else might be affected, however. I'll update here if I find any issues as I'm reviewing.

indigoxela commented 3 years ago

Hm, some tests are failing with your PR.

indigoxela commented 3 years ago

My site is not using the i18n_strings submodule

Interesting, yes that might explain it. Which submodules are you using?

jenlampton commented 3 years ago

From i18n this site is only using Internationalization and Variable translation (I don't know why yet, I'm new to this site)

indigoxela commented 3 years ago

Ooops... so the only thing you use is variable translation - which had to get dropped, because the variables module isn't ported and is unportable.

jenlampton commented 3 years ago

Hm, so i18n doesn't do anything on its own? Well that might make my life a bit easier if I don't need to upgrade this... :)

indigoxela commented 3 years ago

Hm, so i18n doesn't do anything on its own?

It does some small things, but it's mainly providing things for its submodules.

Well that might make my life a bit easier

Hopefully. :wink: Which variables are you using, anyway? Maybe some of those already get handled by core now.

jenlampton commented 3 years ago

Which variables are you using, anyway?

I'm not sure yet :)

indigoxela commented 3 years ago

Which variables are you using, anyway?

I'm not sure yet :)

Try admin/config/regional/i18n/variable (in Drupal) - search for the items that are checked there.

indigoxela commented 3 years ago

Regarding the initial report (PDOException): in rare cases it can happen that the i18n_string module is not on.

This has to be considered in i18n_update_1003. The i18n_string module is the one maintaining that table column. If that module isn't enabled, we can entirely skip over that step, because then no block translations exist, anyway.

@jenlampton your PR doesn't seem the right approach. And some tests are failing, anyway. Feel free to close it. And many thanks for reporting and helping to find the root cause. :+1: