Hi. I just tried updating to 7.x-1.0-beta4 and I get the following:

/Users/myuser/drush-backups/SITE/20121119133835/modules/metatag.
WD metatag: Error loading meta tag data, do the database updates need[error]
to be ran? do the database updates need to be ran? The error occurred
when loading record(s) 0 for the user entity type. The error message
was: SQLSTATE[42S22]: Column not found: 1054 Unknown column
'language' in 'field list'
'all' cache was cleared in                                           [success]
/Library/WebServer/htdocs/SITE/docroot#default
You have pending database updates. Run `drush updatedb` or visit     [warning]
update.php in your browser.
The following updates are pending:

metatag module : 
  7003 -   Add the {metatag}.language field. 
  7004 -   Update all language values in the metatag table, will also resolve problems  created during the release of beta3. 

Do you wish to run all pending updates? (y/n): y
Performed update: metatag_update_7003                                [ok]
Performed update: metatag_update_7004                                [ok]
Performed update: metatag_update_7004                                [ok]
Performed update: metatag_update_7004                                [ok]
Performed update: metatag_update_7004

Ultimately I have to kill the process on the command line in order to break the loop shown above.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

Status: Active » Closed (works as designed)

That's fine, it's looping over all of the records in the {metatag} table and fixing the 'language' value, you probably should just let it finish :)

rickmanelius’s picture

Well crap... sometimes I wish I had a little more patience for what appears to be an infinite loop :)

Thanks Damien!

DamienMcKenna’s picture

Status: Closed (works as designed) » Needs review
FileSize
3.15 KB

You know, it might help if there were more messages about what's going. This patch has an updated description for the update that specifically says "this might take a while" :) and at the end displays how many records were converted.

DamienMcKenna’s picture

Category: bug » support
Status: Needs review » Fixed

Committed.

pdcarto’s picture

It took about an hour for a db update to run on a site with 6600 metatag records. I'm so glad I found this bug report because I certainly would never have recognized that it was working as designed.

DamienMcKenna’s picture

@pcarto: It's a little strange that it ran so slowly, that's only ~2 per second, but I guess if your site is large with lots of entity-related modules running and the database isn't optimized.. Hopefully everything is running well since?

mikeytown2’s picture

Status: Needs review » Fixed

Trying this update out on a site with 200k records and one language. Seems like an infinite loop even though progress is moving :) Guessing this could be short circuited if we know there is only one language, 'und' for most. I have a couple of ideas like searching the url_alias table SELECT * FROM url_alias WHERE language != 'und';, checking if the locale or translation module is enabled, or something else?

DamienMcKenna’s picture

Status: Fixed » Needs review
FileSize
761 bytes

One final, tiny tweak, use drupal_is_cli() instead of php_sapi_name() directly.

mikeytown2’s picture

Status: Fixed » Needs review

I just tested this and it works wonders. Let me know if this is the right idea and I'll roll it as a patch

    // Do quick update if locale and translation modules are not installed.
    if (!module_exists('locale') && !module_exists('translation')) {
      // Assign the global 'no language' value to all metatag rows.
      db_update('metatag')
        ->fields(array('language' => LANGUAGE_NONE))
        ->execute();
      $x = $sandbox['progress'] = $sandbox['max'];
    }
    else {
      // Set default values.
...

Status: Needs review » Needs work

The last submitted patch, metatag-n1844638-8.patch, failed testing.

DamienMcKenna’s picture

Version: 7.x-1.0-beta4 » 7.x-1.x-dev
DamienMcKenna’s picture

Status: Needs work » Needs review

#8: metatag-n1844638-8.patch queued for re-testing.

DamienMcKenna’s picture

@Mikeytown: I think a better alternative would be to manually go through the known entity types and update them from the known schema, manually step through the others?

DamienMcKenna’s picture

Status: Needs review » Needs work

I committed the patch from #8.

Putting this back to Needs Work because we're going to continue working on optimizing the fix.

mikeytown2’s picture

Status: Needs work » Needs review

"known entity types"
FYI this is fatal
entity_load('does_not_exist', array(1));
We had a module define its own entity type, we have since disabled it but meta tags still tries to load it. We manually removed these entries from the metatag table, but this is something that you might want to look into.

I'll look into your idea

mikeytown2’s picture

Entity info is stored in the cache table under entity_info:$langcode where $langcode = $language->language;, $language being a global. http://api.drupal.org/api/drupal/includes!common.inc/function/entity_get...

It appears that this cache table is the only record of what entities are installed and it requires you to know the language. I'm going to attach a patch based off of #9.

ps git hasn't been updated according to git pull.

DamienMcKenna’s picture

@mikeytown2: Thanks for continuing to work on this. What I was really thinking of, though, was jus going through nodes, terms and users, because updating those should be fairly straight forward.

DamienMcKenna’s picture

Status: Needs review » Needs work
DamienMcKenna’s picture

BTW what I'm currently contemplating is adding #1857116: Purge records for entities that are no longer supported to first get rid of unwanted old data, then ditching metatag_update_7004() in favor of two separate functions that first a) correctly handle the core entities, b) loop over all other supported entities just like update_7004 currently does.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
8.26 KB

This needs the replacement metatag_update_7008 to be done, but it should work correctly for all nodes, terms and users.

DamienMcKenna’s picture

FileSize
12.9 KB

This version does several things:

  • It completely scraps update 7004. Please note that a few other updates have been added to purge old, unwanted data.
  • The new update 7008 resets all language values back to an empty string and then manually fixes the meta tag settings for nodes, terms and users.
  • The new update 7009 replaces 7004 in that it fixes the language value for any objects that still don't have a language assigned; this would primarily be for custom entities.
DamienMcKenna’s picture

FileSize
13.22 KB

A tiny update that replaces the $x variable with a $ctr variable.

rickmanelius’s picture

See... and here I thought I was making a big deal out of nothing in #2 :)

DamienMcKenna’s picture

Kristen Pol’s picture

Just patched dev with #22 and got this after running update.php:

Update #7008

    Failed: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'node-6898-' for key 'PRIMARY': UPDATE {metatag} SET language = ''; Array ( ) in metatag_update_7008() (line 325 of /var/www/dev.barefootwine.ca/sites/all/modules/metatag/metatag.install).
DamienMcKenna’s picture

@Kristen: Thanks for that, I'll work on a fix for it.

Kristen Pol’s picture

Thanks @Damien!

DamienMcKenna’s picture

Status: Needs review » Needs work
DamienMcKenna’s picture

@Kristen: Could you please take a quick look through the {metatag} table and let me know what language values show for your node 6898?

DamienMcKenna’s picture

A key problem with the beta4 is that it could potentially lead to multiple records being created for a given entity_type/entity_id pair. I'm kinda wondering if the database really *needs* to store the language value? Are there any D7 multi-language solutions that store multiple records for the same node, or did we needlessly complicate life by adding the language identifier to Metatag?

DamienMcKenna’s picture

@Kristen: Can you please ping me on IRC so I can ask you some questions about your database? I'm in #drupal-seo. Thanks.

DamienMcKenna’s picture

@Kristen: The key thing I'm trying to find out is: for the nodes that have two metatag records, what languages do you have enabled, what language is default, and what is the 'language' value of the correct record?

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
21.36 KB

Argh. This is getting messy.

I decided to try a few changes to the patch in #22:

  • It doesn't reset language values to an empty string, that step is skipped entirely.
  • Code for the core entity types is split into a separate functions, to help avoid hitting the max execution time.
  • Before updating each core entity, it goes through the records to remove duplicate records - if multiple records are found for a single entity with multiple languages it goes through a few checks:
    • It first removes records that have the same values.
    • It then looks to see if there's a matching record for the entity's language value, if so it removes all other ones.
    • It then looks to see if there's a matching record for the site's default language, if so it removes all other ones.
    • It then looks to see if there's a matching record for LANGUAGE_NONE, if so it removes all other ones.
  • It uses the same logic to prune records for custom entities before then updating the records for each one.

There's also a change in the -dev codebase to further help prune redundant records prior to running these steps, so make sure you've got the latest codebase before trying it.

Status: Needs review » Needs work

The last submitted patch, metatag-n1844638-33.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
21.36 KB

Gah.

othermachines’s picture

Category: support » bug

While updating to beta 4 today it took 3 tries to complete the metatag database update because of timeouts. A peek at the server logs show large spikes in CPU usage during the update. Received the following error a couple of times in the logs:

Error loading meta tag data, do the database updates need to be ran? do the database updates need to be ran? The error occurred when loading record(s) 39 for the node entity type. The error message was: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'language' in 'field list'

(Which of course makes sense given I was having trouble completing the update.)

Anyway - eventually completed, server calmed down after awhile. I don't know if it's related but thought I'd report in case it could be helpful. I have no clue what else it could have been.

It's a multisite set-up. The site in question is really small. 'metatag' table has only 123 entries.

** Edit: Was updating with update.php, not Drush.

greggles’s picture

For what it's worth, I used drush to update and didn't have a problem with 7.x-1.0-beta4. I do have locale and entity translation enabled.

DamienMcKenna’s picture

@greggles: Did you try the patch in #1845326? Do you have any problems with data not loading correctly? Do you have any records in {metatag} for the same entity but with different 'language' values?

DamienMcKenna’s picture

@othermachines: Thanks for the info. I think a problem with your situation was that I set the per-iteration limit to 50, which would have been likely to run into a timeout. In the patches here I've changed it to 10, to help ensure it all runs fine.

othermachines’s picture

@DamienMcKenna Ah, gotcha. No harm done, just a head scratcher. Thanks for the great work.

DamienMcKenna’s picture

FileSize
21.4 KB

Rerolled, with some slightly improved comments.

DamienMcKenna’s picture

Priority: Normal » Critical

I'm seriously considering dropping the 'language' field from the primary key, I now seriously think that was a huge mistake and has made this entire thing much more complex than it needs to be. Unless someone can give me a reason why the 'language' value should be a part of the primary key, I'll be removing it in a few days so that this and #1845326: Metatags not loading correctly with beta4 can be finished, allowing us to move on to releasing beta5/rc1.

I would really like to hear from Kristen Pol and Colan, both of whom have lots of experience with internationalization and who's experience I respect in this field, if they can show why the 'language' field must be treated this way then I'll accommodate it but I strongly suspect it should not be.

colan’s picture

@DamienMcKenna: As I don't recall any significant reason for why I made "language" part of the primary key, I'm sure it would be fine if you dropped it (and instead added a standard auto-increment ID field for primary key). However, if you don't want duplicate rows with those same column values, then you'll need to use programming logic to check for these. Hope that helps!

DamienMcKenna’s picture

Status: Needs review » Needs work

@colan: From my research there's no reason to care about the language selection within Metatag other than to tell the translation system the content can be translated. I need to re-examine how Locale and Entity Translation expect the data to behave and update this accordingly.

DamienMcKenna’s picture

Ok, digging into this even more I've realized that yes Entity Translation uses the same node page with different language values for the data fields that are translatable. Back to the drawing board.

DamienMcKenna’s picture

This needs to be completely redone to properly support Entity Translation's use of multiple languages per entity.

DamienMcKenna’s picture

I've worked out how to finish this:

  • User and term records are updated to set 'language' to LANGUAGE_NONE.
  • If Entity Translation is not enabled, update node records to set the 'language' to match {node}.language.
  • If Entity Translation is enabled, don't update node records.
  • If Entity Translation is not enabled, update non-core entity records to set the 'language' to match the response from entity_language().
  • If Entity Translation is enabled, don't update non-core entity records.

I realized that a lot of extra processing was not needed if Entity Translation is not enabled, and that if it is then I believe not touching anything will be the simplest approach, at least for now.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
21.08 KB

Updated patch, untested as I just wanted to get the changes out before I lost them again via git stash collision, needs review.

liquidcms’s picture

is this related to issue i get on numerous sites when running update #7004:

Failed: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'language' cannot be null: UPDATE {metatag} SET language=:db_update_placeholder_0 WHERE (entity_type = :db_condition_placeholder_0) AND (entity_id = :db_condition_placeholder_1) AND (language = :db_condition_placeholder_2) ; Array ( [:db_update_placeholder_0] => [:db_condition_placeholder_0] => field_collection_item [:db_condition_placeholder_1] => 61 [:db_condition_placeholder_2] => en ) in metatag_update_7004() (line 345 of /var/www/cau/sites/all/modules/metatag/metatag.install).

DamienMcKenna’s picture

DamienMcKenna’s picture

FileSize
20.4 KB

Rerolled. Includes a few small improvements to the comments, and to update 7013 so it runs less code when it's not necessary. I also made the decision to, again, not remove any duplicate records if entity_translation is enabled. Incidentally, enough changes were made to the core logic from 7004 (when it was moved to 7013) that the diff now just removes the 7004 function's code rather than trying to move it, which will make future rerolling much easier to manage.

super_romeo’s picture

You need to add {} to 'metatag' table

DamienMcKenna’s picture

FileSize
0 bytes

@super_romeo: Thanks for catching that, can't believe I missed two of them.

DamienMcKenna’s picture

FileSize
20.41 KB

Gah.

DamienMcKenna’s picture

Now that #1845326: Metatags not loading correctly with beta4 has been committed this is next for a final review.

DamienMcKenna’s picture

FileSize
20.4 KB

Rerolled, and corrected a comment.

DamienMcKenna’s picture

Status: Needs review » Fixed

Committed.

DamienMcKenna’s picture

Status: Fixed » Closed (fixed)

Now that Metatag v7.x-1.0-beta5 is out am closing this to keep the issue queue clean.