Please add revisions for entities to metatag

CommentFileSizeAuthor
#107 metatag-n1572474-107.patch20.5 KBDamienMcKenna
#105 metatag-n1572474-105.patch20.5 KBDamienMcKenna
#103 metatag-n1572474-103.patch20.76 KBsylus
#100 metatag-n1572474-99.patch20.37 KBDamienMcKenna
#99 metatag-n1572474-98.patch19.47 KBDamienMcKenna
#97 metatag-n1572474-97.patch18.91 KBDamienMcKenna
#93 metatag-n1572474-93-interdiff.txt1.71 KBKristen Pol
#93 metatag-n1572474-93.patch18.92 KBKristen Pol
#87 metatag.install.info40.05 KBHyperGlide
#83 metatag-n1572474-83.patch18.32 KBDamienMcKenna
#80 metatag-n1572474-78-interdiff.txt5.26 KBDamienMcKenna
#79 9118c053b750f4a06a0015f2ea5fd64b.jpeg88.02 KBHyperGlide
#79 Pasted Graphic.png43.02 KBHyperGlide
#78 metatag-n1572474-78.patch17.85 KBsylus
#76 metatag-n1572474-76.patch17.97 KBsylus
#76 interdiff.patch5.37 KBsylus
#72 metatag-n1572474-72.patch15.07 KBsylus
#53 metatag-n1572474-53.patch13.32 KBDamienMcKenna
#51 metatag-n1572474-51.patch13.32 KBDamienMcKenna
#50 metatag-n1572474-50.patch13.26 KBDamienMcKenna
#49 2013-10-10 01.19.31 pm.png35.23 KBHyperGlide
#47 metatag-n1572474-47.patch12.52 KBDamienMcKenna
#46 metatag_revisions-1572474-46.patch10.29 KBjyee
#45 metatag_revisions-1572474-45.patch10.3 KBjyee
#43 metatag_revisions-1572474-43.patch10.33 KBjyee
#38 metatag-n1572474-36b.patch10.02 KBDamienMcKenna
#22 metatag-n1572474-22.patch13.96 KBDamienMcKenna
#21 metatag-n1572474-21.patch10.49 KBDamienMcKenna
#17 metatag-add_revisions-entity_vid-1572474-17.patch10.17 KBSpartyDan
#15 metatag-add_revisions-entity_vid-1572474-15.patch10.17 KBSpartyDan
#14 metatag-add_revisions-1572474-14.patch9.36 KBSpartyDan
#9 metatag-add_revisions-1572474-9.patch7.21 KBPieIsGood
#6 metatag-add_revisions-1572474-6.patch6.58 KBPieIsGood
#4 metatag-add_revisions-1572474-0.patch5.18 KBPieIsGood
#1 metatag-add_revisions-1572474-0.patch5.18 KBPieIsGood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

PieIsGood’s picture

Status: Active » Needs review
FileSize
5.18 KB

first attempt at a patch be gentle please

Status: Needs review » Needs work

The last submitted patch, metatag-add_revisions-1572474-0.patch, failed testing.

PieIsGood’s picture

Version: 7.x-1.0-alpha4 » 7.x-1.0-alpha6

Oops i put the wrong version of the module. Please retest the patch on alpha 6

PieIsGood’s picture

patch for alpha6 not alpha4

DamienMcKenna’s picture

Status: Needs work » Needs review
PieIsGood’s picture

New patch to fix user add/edit error. Also cleaned up the code a bit

SpartyDan’s picture

I am using metatag alpha6. When I revert a node to a previous version the metatag information is not reverted. Does this patch address that problem?

PieIsGood’s picture

I'll run some tests and let you know. Hopefully I'll have an answer by this weekend.

PieIsGood’s picture

New patch that addresses php warning on function metatag_metatags_delete

PieIsGood’s picture

Hey Dan,

The patch loads the tags based on revision. So when a node is reverted it will load the tags on the reverted version. Let me know if you run into any problems.

DamienMcKenna’s picture

Status: Needs review » Needs work

This needs to be rerolled.

DamienMcKenna’s picture

@pieisgood: Any luck rerolling the patch? I'd really like to include this functionality in the next release. Thanks :)

SpartyDan’s picture

Status: Needs work » Active

Damien, I just contributed my first core patches today. I am going to re-roll this patch tonight but it might take a while. - Dan

SpartyDan’s picture

Version: 7.x-1.0-alpha6 » 7.x-1.x-dev
Status: Active » Needs work
FileSize
9.36 KB

I re-rolled the patch and applied to -dev. I am getting an error in the message div at the top of the page:

Error loading meta tag data, do the database updates need to be run?

I have run the updates but still get the error.

I can't figure out what I'm missing or doing wrong. Any suggestions would be appreciated. I'm very new to contributing patches and would like to learn and be more useful.

SpartyDan’s picture

Status: Needs work » Needs review
FileSize
10.17 KB

Patch re-rolled for 7.x-1.x-dev

$vid and $vids changed to $entity_vid and $entity_vids so that they match module's naming convention.

Status: Needs review » Needs work

The last submitted patch, metatag-add_revisions-entity_vid-1572474-15.patch, failed testing.

SpartyDan’s picture

Status: Needs work » Needs review
FileSize
10.17 KB

fixed incorrect variable name

DamienMcKenna’s picture

In the interest of having a stable API for 1.0, this must be included before we get that far.

DamienMcKenna’s picture

Status: Needs review » Needs work
Issue tags: +D7 stable release blocker

The last submitted patch, metatag-add_revisions-entity_vid-1572474-17.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
10.49 KB

Rerolled.

DamienMcKenna’s picture

FileSize
13.96 KB

This fixes metatag_field_attach_delete_revision(), renames the entity_vid field to the standard "revision_id", and adds a note to the README.txt file. I've tested it and it worked well both with standard Drupal core and with the Revisioning module.

DamienMcKenna’s picture

Status: Needs review » Fixed

I've tested this with a multilingual structure using EntityTranslation and it worked as advertised.

Committed! Thank you both for your help with this!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

HyperGlide’s picture

Status: Closed (fixed) » Needs review

I have tested the latest dev 2013-Aug-06 which included this patch after running update.php from the 7.x-1.0-beta7 have the following observations / bugs our site is using workbench moderation:

  1. There appears to be no upgrade path - for all the entries within the metatag table the values set for revision_ids in all cases was 0.
  2. This renders all existing data to be unaccessible.
  3. For nodes or other entities with there are "n" revisions the metatag table only shows one entry and does not populate the full table for all revisions in the db. (I would suggest that all revisions be assigned the same value as current. Or if no value then use current.
  4. If we manually edit the revision_ids value in the db we could not see the data in the node/edit
  5. If we edit an existing node and directly publish it the values keep.
  6. If we edit an existing row and save as a non published state it drops all rows int he db for that node.
  7. If we create a new node all works well.

Let me know of any questions of follow ups.

DamienMcKenna’s picture

Status: Needs review » Needs work

Thanks for bringing these problems to our attention. I'm going to work through them one a time.

DamienMcKenna’s picture

Priority: Normal » Critical

This is now critical.

HyperGlide’s picture

@DamienMcKenna np. you can ping me when ever you need and I would be happy to test a patch on our site.

wizonesolutions’s picture

Yeah, when using Revisioning (at least I think that's the reason), Open Graph tags use their defaults and not the configured value for the particular revision. Viewing them through node/% works as expected.

4fs’s picture

Just to add to the discussion. I am also having a similar experience as HyperGlide. It is rather a major issue.

I am running Metatag 7.x-1.0-beta7+23-dev (I think this version has one patch in it). My problem is that I have panel nodes (biggest issue) and views which do not display my custom metatags on a fairly basic multilingual website setup. All modules/core are up to date except media of which I am having some assuming non related issues when updating.

I have tested the latest dev (2013-Aug-06) and dev version before that and I get the following error "Error loading meta tag data, do the database updates need to be run?". At this point, before update.php it appears as though my metatags have allready been removed from he db on the node edit page. I run update.php and they remain removed/deleted.

I hope this helps.!

FiNeX’s picture

I've installed metatags -dev and I've fallen into this bug. Currently I'm not using revisions so I've manually updated the revision_id column with the entity_id values, fortunatly I were not working on a production website :-)

HyperGlide’s picture

Besides writing a patch is there anything else that can be done to help expedite this issue?

HyperGlide’s picture

DamienMcKenna’s picture

FYI #2082539: Warning message metatag.revision_id is part of the primary key but is not specified to be 'not null'. has been tested (on MySQL only..) and committed, now on with the show for this issue.

DamienMcKenna’s picture

I reverted #2082539: Warning message metatag.revision_id is part of the primary key but is not specified to be 'not null'. because the revision_id field is actually supposed to allow NULL values for entities that don't support revisions, e.g. users.

DamienMcKenna’s picture

Status: Needs work » Needs review

This adds an update script (metatag_update_7017) to update all metatag records so they have the correct revision_id values. It also changes how metatag_metatags_load_multiple() works a little bit so that it should work correctly with entities that don't have revision IDs, and fix problems with metatag_entity_update() when updating records.

I'd appreciate some help in testing it. Thanks.

DamienMcKenna’s picture

For anyone testing this, make sure you've got the latest -dev release as I identified and fixed some other small problems today.

DamienMcKenna’s picture

FileSize
10.02 KB

Oh yeah, THE PATCH FILE.

_12345678912345678’s picture

#38: metatag-n1572474-36b.patch queued for re-testing.

HyperGlide’s picture

Looking to test and hope confirm.

1 - Install Dev.
2 - There is a pending updated - 7015 - Add the revision_id from the entity into metatag schema, adjust the primary keys accordingly.
3 - Do NOT run this update.php
4 - Apply the patch.
5 - Run update.php

Thank you.

_12345678912345678’s picture

I must have missed something on configuration on this one. Im using dreditor and I tried to install the patch with

git apply -v metatag-n1572474-36b.patch

came up with
fatal: unrecognized input

let me know what I did wrong if you know. I would like to test this one. Thanks.

HyperGlide’s picture

@greenroomwebdesign are you using a git repo or just a dl of the module?

I was able to patch using:

patch -p1 < metatag-n1572474-36b.patch

Question is do you run update 7015 then patch or patch and then run update.php?

After patching update.php reports:

7015 - Add the revision_id from the entity into metatag schema, adjust the primary keys accordingly.
7016 - This was removed.
7017 - Update the revision ID for each record. This may take some time.

Ran Update.PHP after patching file. Had the following error:

The following updates returned messages

metatag module

Update #7017
Failed: PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'base.language' in 'where clause': SELECT base.fid AS fid, base.uid AS uid, base.filename AS filename, base.uri AS uri, base.filemime AS filemime, base.filesize AS filesize, base.status AS status, base.timestamp AS timestamp, base.type AS type FROM {file_managed} base WHERE (base.fid IN (:db_condition_placeholder_0)) AND (base.language = :db_condition_placeholder_1) ; Array ( [:db_condition_placeholder_0] => 4796 [:db_condition_placeholder_1] => und ) in EntityCacheControllerHelper::entityCacheLoad() (line 95 of sites/all/modules/entitycache/entitycache.module).

Ran update.php again same error.

Disabled entity cache module and testing again.

Failed with:

metatag module

Update #7017
Failed: PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'base.language' in 'where clause': SELECT base.fid AS fid, base.uid AS uid, base.filename AS filename, base.uri AS uri, base.filemime AS filemime, base.filesize AS filesize, base.status AS status, base.timestamp AS timestamp, base.type AS type FROM {file_managed} base WHERE (base.fid IN (:db_condition_placeholder_0)) AND (base.language = :db_condition_placeholder_1) ; Array ( [:db_condition_placeholder_0] => 4796 [:db_condition_placeholder_1] => und ) in DrupalDefaultEntityController->load() (line 196 of includes/entity.inc).
jyee’s picture

Reroll of #38 for dev revision 9bd40e4 (Sept 29, 2013)

jyee’s picture

The reroll produces this error on update.php:

The following updates returned messages
metatag module
Update #7017
Failed: PDOException: SQLSTATE[HY093]: Invalid parameter number: no parameters were bound in metatag_update_7017() (line 1079 of /Users/jyee/Documents/Projects/corpdrupal/sites/all/modules/metatag/metatag.install).

jyee’s picture

#38 (and the reroll in #43) had a couple issues around db_query() and the subsequent loop. This patch should resolve those and from my testing, it doesn't seem to matter if you run update.php with dev, apply patch, then re-run update.php for the 7017 update or if you apply the patch and run everything with one update.php.

jyee’s picture

Minor tweak to use standard drupal variable names. Functionality is the same as #45.

DamienMcKenna’s picture

FileSize
12.52 KB

This is my latest effort to resolve this. It has been rerolled to adjust for hook_update_N changes, and updated so that the revision_id is always set to 0 instead of NULL. I need to do more testing of this, especially with complicated scenarios where both languages and revisions are used, so please only test it on a backup of your site.

HyperGlide’s picture

Thanks for the updates, I would also ask that we test it with workbench_moderation. If you have a particular testing procedure please share. (I will try to test but leaving for several day holiday so will be more difficult)

HyperGlide’s picture

Status: Needs review » Needs work
FileSize
35.23 KB

Test patch from no. 47.

  1. Downloaded latest dev 2013-Oct-01.
  2. Downloaded and cleanly patch from no. 47
  3. Ran update.php
  4. update

  5. Update failed with the following error:
  6. Update #7018
    Failed: PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'base.language' in 'where clause': SELECT base.fid AS fid, base.uid AS uid, base.filename AS filename, base.uri AS uri, base.filemime AS filemime, base.filesize AS filesize, base.status AS status, base.timestamp AS timestamp, base.type AS type FROM {file_managed} base WHERE (base.fid IN (:db_condition_placeholder_0)) AND (base.language = :db_condition_placeholder_1) ; Array ( [:db_condition_placeholder_0] => 4796 [:db_condition_placeholder_1] => und ) in EntityCacheControllerHelper::entityCacheLoad() (line 95 of /sites/all/modules/entitycache/entitycache.module).
  7. Ran update again and received the same message, as noted above.
DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
13.26 KB

This version improves upon #47 by only grabbing the $language and $revision_id values for entities that support them, which will resolve the problem reported in #49.

DamienMcKenna’s picture

FileSize
13.32 KB

This is a slight improvement over #50 in that it tracks the field names for the $revision_id and $language values per entity type, just to make it a little more correct.

HyperGlide’s picture

Status: Needs review » Needs work

Tested #51 and returned:

Update #7018
Fixed the revision_id values for @count {metatag} records.
Failed: PDOException: SQLSTATE[HY093]: Invalid parameter number: no parameters were bound in metatag_update_7018() (line 1100 of /modules/metatag/metatag.install).
DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
13.32 KB

Gah. Typo. Lets try this.

HyperGlide’s picture

Tested #52 and returned:

Update #7018
Fixed the revision_id values for 1 {metatag} records.
Failed: PDOException: SQLSTATE[HY093]: Invalid parameter number: no parameters were bound in metatag_update_7018() (line 1100 of /modules/metatag/metatag.install).
HyperGlide’s picture

Status: Needs review » Needs work
DamienMcKenna’s picture

Are you sure you're applying the patch against the lastest -dev version? Line 1100 of metatag.install is:

                $record->language = $node->language;
HyperGlide’s picture

@DamienMcKenna - Yes was applying and testing on the latest dev.

After some more testing we have identified the following issues, that are contributing to the error reported in #54.

Our site has the core "locale" module and contrib. "workench_moderation" enabled.

  1. Site specific, but may impact other users. On several tables such as field_data_bodywe have duplicate entries for the same "entity_id" and "version_id" the primary difference is that the languages are not the same. During the course of the testing today, I disabled and uninstalled the local module and then set all languages DB wide to "und". Including the metatag table. Then running the update.php script again did not product any better results. Perhaps the update scripts is set to confirm there is only 1 revision_id for each entity_id, which is sound, but in our case and perhaps other there is corruption in the DB that causes the update to fail.

  2. Perhaps another 'bug' in metatag that was identified when testing further was. After I started to delete 'nodes' that fit the condition stated above. In all tables BUT metatag the data was removed for all entries. This is including the duplicates as described above. However within the metatag table the values where not deleted and I had manually drop those rows.


    Testing further - I spun up a new site without workbench_moderation and confirmed that this was an issue where revisions are not deleted fro the metatag table when a node is deleted.

Certainly item 1 above is not ideal for testing this patch, I do suspect that we are not the only site that has this condition. We are in the process of cleaning up the DB after this discovery.

Thank you for you continued help on getting this patch resolved and committed.

DamienMcKenna’s picture

DamienMcKenna’s picture

sylus’s picture

Just wanted to chime in here as well stating experiencing the exact same results as HyperGlide.

Again using locale, entity_translation, and workbench moderation.

Tested on a fresh drush make install using:


projects[metatag][version] = 1.x-dev
projects[metatag][subdir] = contrib
projects[metatag][type] = module
projects[metatag][download][type] = git
projects[metatag][download][revision] = 8a05eec
projects[metatag][download][branch] = 7.x-1.x
projects[metatag][patch][1572474] = http://drupal.org/files/metatag-n1572474-53.patch

sylus’s picture

HyperGlide’s picture

@sylus Thanks for the follow up. To confirm are you experiencing the condition No. 1 I outlined above?

On several tables such as field_data_bodywe have duplicate entries for the same "entity_id" and "version_id" the primary difference is that the languages are not the same.

I am not certain this is a metatag issue or workbench_moderation. I believe it is related to WBM and locale.

sylus’s picture

I am pretty certain this is a metatag issue but yeah not knowledgeable enough in this area to say for certain.

I was wondering if this issue should be classified as a critical bug? It is stated we have support for entity translation but without this issue it isn't necessarily fully complete.

Anyways no doubt workbench is playing a key problem here.

How can we move this issue forward?

HyperGlide’s picture

@stylus it is marked critical b/c of #25

There appears to be no upgrade path - for all the entries within the metatag table the values set for revision_ids in all cases was 0.

sylus’s picture

Removed comment as assumption was incorrect.

sylus’s picture

Removed comment as assumption was incorrect.

DamienMcKenna’s picture

I've been a little distracted the past few weeks so haven't gotten any further yet, but I thank you both for keeping at it.

For now I want to work on the main issue of making revisions work correctly with core and then with EntityTranslation too, once those two are working correctly we can focus on the additional step of ensuring nothing breaks when Workbench Moderation.

DamienMcKenna’s picture

BTW in metatag.test I've documented a list of 25+ scenarios to be manually tested in lieu of having a full set of tests, feel free to help suggest other scenarios that should be tested too.

HyperGlide’s picture

@DamienMcKenna thanks for the update. Any ETA on the next patch to test?

sylus’s picture

Just wanted to offer some further information as have been playing around with this.

First thing I noticed is possibly an improper use of array_key_exists:

    if (empty($revision_ids) || array_key_exists($record->revision_id, $revision_ids)) {
      $metatags[$record->entity_id][$record->revision_id][$record->language] = unserialize($record->data);
    }

I think it should be:

    if (empty($revision_ids) || in_array($record->revision_id, $revision_ids)) {
      $metatags[$record->entity_id][$record->revision_id][$record->language] = unserialize($record->data);
    }

Secondly the problem actually has nothing to do with workbench_moderation just with revisions themselves not having a language field. Essentially metatags loads the tags based on revision but if you are using entity translation the $node->vid will increment for every save regardless of language.

sylus’s picture

FileSize
15.07 KB

Removed comment as assumption was incorrect.

Elijah Lynn’s picture

Status: Needs work » Needs review
HyperGlide’s picture

Tested #72 and the same condition as reported in 57#1 above.

Update #7018
Fixed the revision_id values for 11 {metatag} records.
Failed: PDOException: SQLSTATE[HY093]: Invalid parameter number: no parameters were bound in metatag_update_7018() (line 1100 of /modules/metatag/metatag.install).
HyperGlide’s picture

Status: Needs review » Needs work
sylus’s picture

FileSize
5.37 KB
17.97 KB

Did some more work on this and added an interdiff between the patch in: #53

Patch adds the following:

a) Fix for using array_key_exists where it should be in_array
b) Need to pass the old_vid to metatag_metatag_save so revisions in every language can be in sync
c) Need to add logic for workbench moderation as it calls node_save twice. The following issue is relevant: #1879482: hook_node_update() fires twice and cause abnormal results

HyperGlide’s picture

@sylus thanks for the update.

So are you saying that #76 is a new patch that requires testing?

sylus’s picture

FileSize
17.85 KB

Updating the patch since hook_entity_insert shouldn't be passing an old_vid.

Re: the above comment I think the workbench moderation code is relevant to you as will bypass the second save.

HyperGlide’s picture

Tested with similar results:

error

error

DamienMcKenna’s picture

Issue tags: +D7onD7
FileSize
5.26 KB

FINALLY!!! getting back to this one today.

First off, good catch on the array_key_exists vs in_array thing, that was definitely a bug.

Here's an interdiff between 53 and 78, to help debugging.

DamienMcKenna’s picture

Something we need to check:

  • Should the code in metatag_metatags_save that deletes any possible old data if $metatags[$langcode] is empty limit it to the revision?
DamienMcKenna’s picture

@HyperGlide: Can you please tell me what you see on line 1100 of metatag.install? I see the following:

            $metatags = db_query("SELECT data
                FROM {metatag}
                WHERE entity_type = :entity_type
                AND entity_id = :entity_id
                AND language = :language",
              array(
                ':entity_type' => $entity_type,
                ':entity_id' => $entity_id,
                ':language' => $language,
              ))->execute();

For me line 1100 is the execute() line.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
18.32 KB

Minor adjustments to the comments to make them fit better in 80 characters.

DamienMcKenna’s picture

While working on #2152043: Integration with Devel Generate with patch 83 running, generating a node results in the following error:
Notice: Undefined property: stdClass::$old_vid in metatag_entity_update() (line 698 of metatag.module).

DamienMcKenna’s picture

To help with this, I've spent a good chunk of today working on this: #2152043: Integration with Devel Generate

DamienMcKenna’s picture

The Devel Generate integration has been committed, which will help debugging this.

HyperGlide’s picture

FileSize
40.05 KB

@DamienMcKenna -- Tested again on a clean site clone.

Running '7.x-1.x-dev - 2013-Dec-08 - Development "
Per modules page -- 7.x-1.0-beta7+61-dev
Applied patch #83 cleanly
Line 1100 of metatag.install

              ))->execute();
            foreach ($revisions as $vid -> $revision) {
              if ($vid != $revision_id) {
                $node = node_load($entity_id, $vid);
                $record = new StdClass();
                $record->entity_type = 'node';
                $record->entity_id = $node->nid;
                $record->revision_id = $node->vid;
                $record->language = $node->language;
                $record->data = $metatags->data;
                drupal_write_record('metatag', $record);
              }
            }
          }
        }

On 1091 I see

 $metatags = db_query("SELECT data
                FROM {metatag}
                WHERE entity_type = :entity_type
                AND entity_id = :entity_id
                AND language = :language",
              array(
                ':entity_type' => $entity_type,
                ':entity_id' => $entity_id,
                ':language' => $language,
              ))->execute();

I am going to attach a copy of the metatag.install as well.

Did not run any updates via update.php as your code base seems different. Holding for your reply.

jyee’s picture

#83 also cleanly applied for me. I get the same "PDOException: SQLSTATE[HY093]: Invalid parameter number: no parameters were bound in metatag_update_7018() (line 1100 of /sites/all/modules/metatag/metatag.install)" error. Line 1100 is " ))->execute();"

jyee’s picture

Also, for some reason, I'm seeing $node->metatags with the correct data, but the page does not display them. Node pages appear to be loading the default metatags.

edit: this issue was unrelated to metatag and was due to another module clobbering the $page['content'] array.

HyperGlide’s picture

Status: Needs review » Needs work
jyee’s picture

The function _metatag_isdefaultrevision() in patch #83 doesn't seem quite right and I think it breaks metatags for sites that do not have workbench_moderation.

In metatag_entity_insert(), line 651, we find this code (and similar to code in metatag_entity_update():

// Support for Metatag + Workbench Moderation.
if ($entity_type == 'node' && _metatag_isdefaultrevision($entity)) {
  return;
}
metatag_metatags_save($entity_type, $entity_id, $revision_id, $entity->metatags, $langcode);

The problem is that _metatag_isdefaultrevision() has this logic:

if (module_exists('workbench_moderation') && workbench_moderation_node_type_moderated($entity->type) === TRUE && empty($entity->workbench_moderation['updating_live_revision'])) {
  return FALSE;
}
return TRUE;

Which means that if workbench moderation is not enabled, it will return TRUE causing metatag_entity_insert/update to return before ever saving metatags.

Kristen Pol’s picture

I just updated to latest dev and applied patch from #83 and it appears to work for nodes with moderation but now nodes without moderation don't work (the meta tags aren't saving). Perhaps due to the observation by @jyee on #91.

Kristen Pol’s picture

Status: Needs work » Needs review
FileSize
18.92 KB
1.71 KB

I updated the logic slightly to get it to work for non-WBM nodes. I tested these use cases:

1) WBM enabled and WBM node with only one revision

2) WBM enabled and WBM node with published version and a revision

3) WBM enabled and non-WBM node

4) WBM disabled and non-WBM node

I also added some code so that the metatags show up on the draft page so that you can see what the revision's metatags are before publishing.

HyperGlide’s picture

@Kristen Pol Thanks.

I tested #93.

Failed: PDOException: SQLSTATE[HY093]: Invalid parameter number: no parameters were bound in metatag_update_7018() (line 1100 of /modules/metatag/metatag.install).

metatag.install

        // Nodes can have multiple revisions, so create new {metatag} records
        // for each of the other revisions.
        if ($entity_type == 'node') {
          $revisions = node_revision_list($entity);
          if (count($revisions) > 1) {
            $metatags = db_query("SELECT data
                FROM {metatag}
                WHERE entity_type = :entity_type
                AND entity_id = :entity_id
                AND language = :language",
              array(
                ':entity_type' => $entity_type,
                ':entity_id' => $entity_id,
                ':language' => $language,
1100         ))->execute();
            foreach ($revisions as $vid -> $revision) {
              if ($vid != $revision_id) {
                $node = node_load($entity_id, $vid);
                $record = new StdClass();
                $record->entity_type = 'node';
                $record->entity_id = $node->nid;
                $record->revision_id = $node->vid;
                $record->language = $node->language;
                $record->data = $metatags->data;
                drupal_write_record('metatag', $record);
              }
            }
          }
        }
Kristen Pol’s picture

Thanks @HyperGlide... hm... I didn't change any of the install code and I didn't get that error. Guess Damien will need to comment on that.

HyperGlide’s picture

@Kristen Pol - Np.

We resolved the issue first noted in #57.

Retested with the following results:

Fixed the revision_id values for 11 {metatag} records.
Failed: PDOException: SQLSTATE[HY093]: Invalid parameter number: no parameters were bound in metatag_update_7018() (line 1100 of modules/metatag/metatag.install).

Is there a way to increase the output for debugging purposes?

DamienMcKenna’s picture

@Hyperglide: I think the error you were seeing was because db_query() doesn't need an execute() call, sorry about that.

This patch is identical to Kristin's from #93 only with a fix for the db_query() mistake. I haven't tested it yet, am about to do that.

DamienMcKenna’s picture

DamienMcKenna’s picture

FileSize
19.47 KB

This appears to have fixed the problem of records not being deleted when an entity object is being deleted.

DamienMcKenna’s picture

FileSize
20.37 KB

This version loads the appropriate meta tags when viewing a node revision using the core node revision system.

DamienMcKenna’s picture

I've tested the patch from #100 using core and entity_translation across various scenarios, it seems to work fine so far. I'd like to hear back from others before I commit this, just in case it doesn't catch a specific scenario.

HyperGlide’s picture

I tested #100 with no success.

When testing via update.php the progress bar never process and no error is reported.

When testing via drush updatedb --verbose

The following is reverted back:

Initialized Drupal 7.22 root directory at /data/disk/dev/static/wht-v15                                                    [notice]
Initialized Drupal site meta.11.domain.com at sites/meta.11.domain.com                        [notice]
Use of undefined constant MASKEDINPUT_DOWNLOAD_URI - assumed 'MASKEDINPUT_DOWNLOAD_URI' masked_input.drush.inc:10                                                                [notice]
Load alias @server_master                                                                         [notice]
Loading mysql driver for the db service                                                                     [notice]
Loading nginx driver for the http service                                                                      [notice]
Loading nginx driver for the cdn service                                                                          [notice]
Loading nginx driver for the cdn service                                                                         [notice]
The following updates are pending: 
metatag module : 
  7018 -   Update the revision ID for each record. This may take some time. 

Do you wish to run all pending updates? (y/n): y
Initialized Drupal 7.22 root directory at /data/disk/dev/static/wht-v15                                             [notice]
Initialized Drupal site meta.11.domain.com at sites/meta.11.domain.com                                          [notice]
Use of undefined constant MASKEDINPUT_DOWNLOAD_URI - assumed 'MASKEDINPUT_DOWNLOAD_URI' masked_input.drush.inc:10                                                                [notice]
Load alias @server_master                                                                   [notice]
Loading mysql driver for the db service                                                                       [notice]
Loading nginx driver for the http service                                                                        [notice]
Loading nginx driver for the cdn service                                                                         [notice]
Loading nginx driver for the cdn service                                                                        [notice]
Executing metatag_update_7018                                                                                   [notice]
WD metatag: Update 7018: 1645 records to update.                                       [info]
Undefined variable: revision metatag.install:1101                                                             [notice]
Drush command terminated abnormally due to an unrecoverable error.                                             [error]
Error: Cannot access empty property in /data/disk/dev/static/wht-v15/sites/meta.11.domin.com/modules/metatag/metatag.install, line 1101
sylus’s picture

FileSize
20.76 KB

@HyperGlide your last comment pointed out it errored on line 1101 at this code:

foreach ($revisions as $vid -> $revision) {

This is incorrect syntax and should be:

foreach ($revisions as $vid => $revision) {

I have updated the patch with the fix how does it work for you now?

I am anticipating this will resolve your problems.

Confirming expected functionality is working for me with workbench moderation. If HyperGlide confirms issues is fixed when invoking hook_update_n updates lets RTBC this!

HyperGlide’s picture

@sylus++

Running from Drush.

There where several:

Undefined property: DatabaseStatementBase::$data metatag.install:1109

But, the DB is updated!

WD metatag: Update 7018: 1643 records were updated in total.      [info]
Update 7018: 1643 records to update.                      [status]
Update 7018: 1643 records were updated.                                   [status]
Command dispatch complete                               [notice]
Finished performing updates.                                      [ok]
Command dispatch complete                                   [notice]

I ran this on the same site as yesterday with the failed update. While it should not matter I would like to run it again on a clean copy to make sure no issues. Along with run some workflow testing to ensure nothing breaks.

One immediate observation is the table grew significantly from 1,647 records to 13,225.

Wonderful!

For the none coder what is the difference between -> and =>

Thanks and lets get this committed before the clock strikes midnight on 12/31/13.

DamienMcKenna’s picture

FileSize
20.5 KB

Thanks for the fix on 103, sylus.

This is updated to only bother with the inner foreach() loop if $metatag->data exists.

Status: Needs review » Needs work

The last submitted patch, 105: metatag-n1572474-105.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
20.5 KB

#facepalm.

HyperGlide’s picture

Testing #107.

Update #7018
Fixed the revision_id values for 1655 {metatag} records.
DamienMcKenna’s picture

@Hyperglide: That's excellent! Did you verify the records were correct / workable?

aimeerae’s picture

Status: Needs review » Reviewed & tested by the community

Hi @Damien, I work with Kristen Pol. :) I just applied the most recent patch and things look like they are working nicely. Thank you for the quick attention!

DamienMcKenna’s picture

@Aimee: Thanks for the review, I'm really glad we finally got it to the point of working correctly for you. I'm just waiting to hear back from HyperGlide on whether the data continued to work correctly after the upgrade, or anyone else who might have tested it on an existing site with Entity_Translation, and I'll commit it!

HyperGlide’s picture

@DamienMcKenna - Testing today. will revert back very soon. But so far all things are looking good. Please stand by.

HyperGlide’s picture

All looks good. Agree RTBC! Roll out a beta-8 or _____? HNY!

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed
  ____ ___  __  __ __  __ ___ _____ _____ _____ ____  _ 
 / ___/ _ \|  \/  |  \/  |_ _|_   _|_   _| ____|  _ \| |
| |  | | | | |\/| | |\/| || |  | |   | | |  _| | | | | |
| |__| |_| | |  | | |  | || |  | |   | | | |___| |_| |_|
 \____\___/|_|  |_|_|  |_|___| |_|   |_| |_____|____/(_)

__   __ _ __   ___ 
\ \ / // \\ \ / / |
 \ V // _ \\ V /| |
  | |/ ___ \| | |_|
  |_/_/   \_\_| (_)

Thank you all for your help working on this, this is the largest & most important fix prior to beta8, which I'll release later this week. Happy New Year! :-D

Elijah Lynn’s picture

DamienMcKenna’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

eelkeblok’s picture

I wouldn't dare reopening this issue (well done everyone), but I would like to note that correcting the revision IDs can take a *very* long time on a site with a lot of nodes (41K on the site I am working on). I am currently pondering a way to do the update "offline" (or maybe "online" is a better way of saying it; I mean, outside the regular update process), because it is not an option to leave the site in maintenance mode for the duration of this update script.

eelkeblok’s picture

As a follow-up to my previous comment, I came up with a "trick". Please proceed at your own risk.

Create a module with the following stuff (you may need to tweak stuff here and there as I took this out of a glue code module that has all sorts of loose ends for this project.

Create an implementation of hook_menu and create a callback to a confirmation form:

  $items['admin/config/mymodule/metatag-update'] = array(
    'title' => 'Perform meta tag update 7018',
    'description' => "Metatag module's hook_update_7018() takes a long time to execute. During the normal update process, the site would have to be online, which is not acceptable.",
    'page callback' => 'drupal_get_form',
    'page arguments' => array('mymodule_metatag_update'),
    'access arguments' => array('administer site configuration'),
  );

Create the following functions to create the form and have its submit handler call the update hook through batch API:

/**
 * Callback to generate a confirmation form to perform metatag update 7018.
 */
function mymodule_metatag_update($form, &$form_state) {
  return confirm_form(array(), t('Are you sure you want to run the metatag update?'), 'admin/config/mymodule');
}

/**
 * Submit function for mymodule_metatag_update
 */
function mymodule_metatag_update_submit($form, &$form_state) {
  $batch = array(
    'operations' => array(),
    'title' => t('Performing updates to metatag data...'),
    'init_message' => t('Getting ready to perform updates to metatag data...'),
    'error_message' => t('Metatag update has encountered an error.'),
    'file' => drupal_get_path('module', 'spn_bespoke') . '/spn_bespoke.admin.inc',
  );

  $batch['operations'][] = array('mymodule_metatag_update_operation', array());

  batch_set($batch);
}

/**
 * Operation function for rebuild taxonomy index batch
 */
function mymodule_metatag_update_operation(&$context) {
  module_load_include('install', 'metatag', 'metatag');

  metatag_update_7018($context['sandbox']);

  $context['finished'] = $context['sandbox']['#finished'];
  $context['message'] = t(
    'Processed @current out of @total.',
    array(
      '@current' => $context['sandbox']['progress'],
      '@total' => $context['sandbox']['max'],
    )
  );
}

After having enabled your module (or cleared your cache when adding it to an existing gluecode module), run the update process as usual, but abort the process when it hits the 7018 update.

Edit the schema_version entry in the system table for metatag to say 7018, and continue the update process. You may need to take your site out of maintenance mode afterwards, not sure.

Your site will now be online again, but which lots of broken metatags. Visit the URL, confirm, and watch the progress bar.

Again, proceed at your own risk, I created this only just now, so PLEASE test this thoroughly before deploying on a production site.