Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Please add revisions for entities to metatag
Comment | File | Size | Author |
---|---|---|---|
#107 | metatag-n1572474-107.patch | 20.5 KB | DamienMcKenna |
Comments
Comment #1
PieIsGood CreditAttribution: PieIsGood commentedfirst attempt at a patch be gentle please
Comment #3
PieIsGood CreditAttribution: PieIsGood commentedOops i put the wrong version of the module. Please retest the patch on alpha 6
Comment #4
PieIsGood CreditAttribution: PieIsGood commentedpatch for alpha6 not alpha4
Comment #5
DamienMcKennaComment #6
PieIsGood CreditAttribution: PieIsGood commentedNew patch to fix user add/edit error. Also cleaned up the code a bit
Comment #7
SpartyDan CreditAttribution: SpartyDan commentedI 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?
Comment #8
PieIsGood CreditAttribution: PieIsGood commentedI'll run some tests and let you know. Hopefully I'll have an answer by this weekend.
Comment #9
PieIsGood CreditAttribution: PieIsGood commentedNew patch that addresses php warning on function metatag_metatags_delete
Comment #10
PieIsGood CreditAttribution: PieIsGood commentedHey 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.
Comment #11
DamienMcKennaThis needs to be rerolled.
Comment #12
DamienMcKenna@pieisgood: Any luck rerolling the patch? I'd really like to include this functionality in the next release. Thanks :)
Comment #13
SpartyDan CreditAttribution: SpartyDan commentedDamien, I just contributed my first core patches today. I am going to re-roll this patch tonight but it might take a while. - Dan
Comment #14
SpartyDan CreditAttribution: SpartyDan commentedI 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.
Comment #15
SpartyDan CreditAttribution: SpartyDan commentedPatch 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.
Comment #17
SpartyDan CreditAttribution: SpartyDan commentedfixed incorrect variable name
Comment #18
DamienMcKennaIn the interest of having a stable API for 1.0, this must be included before we get that far.
Comment #19
DamienMcKenna#17: metatag-add_revisions-entity_vid-1572474-17.patch queued for re-testing.
Comment #21
DamienMcKennaRerolled.
Comment #22
DamienMcKennaThis 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.
Comment #23
DamienMcKennaI've tested this with a multilingual structure using EntityTranslation and it worked as advertised.
Committed! Thank you both for your help with this!
Comment #25
HyperGlide CreditAttribution: HyperGlide commentedI 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:
Let me know of any questions of follow ups.
Comment #26
DamienMcKennaThanks for bringing these problems to our attention. I'm going to work through them one a time.
Comment #27
DamienMcKennaThis is now critical.
Comment #28
HyperGlide CreditAttribution: HyperGlide commented@DamienMcKenna np. you can ping me when ever you need and I would be happy to test a patch on our site.
Comment #29
wizonesolutionsYeah, 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.Comment #30
4fs CreditAttribution: 4fs commentedJust 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.!
Comment #31
FiNeX CreditAttribution: FiNeX commentedI'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 :-)
Comment #32
HyperGlide CreditAttribution: HyperGlide commentedBesides writing a patch is there anything else that can be done to help expedite this issue?
Comment #33
HyperGlide CreditAttribution: HyperGlide commentedRelated Issue #2082539: Warning message metatag.revision_id is part of the primary key but is not specified to be 'not null'.
Comment #34
DamienMcKennaFYI #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.
Comment #35
DamienMcKennaI 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.
Comment #36
DamienMcKennaThis 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.
Comment #37
DamienMcKennaFor anyone testing this, make sure you've got the latest -dev release as I identified and fixed some other small problems today.
Comment #38
DamienMcKennaOh yeah, THE PATCH FILE.
Comment #39
_12345678912345678 CreditAttribution: _12345678912345678 commented#38: metatag-n1572474-36b.patch queued for re-testing.
Comment #40
HyperGlide CreditAttribution: HyperGlide commentedLooking 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.
Comment #41
_12345678912345678 CreditAttribution: _12345678912345678 commentedI 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.
Comment #42
HyperGlide CreditAttribution: HyperGlide commented@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:
Ran Update.PHP after patching file. Had the following error:
Ran update.php again same error.
Disabled entity cache module and testing again.
Failed with:
Comment #43
jyee CreditAttribution: jyee commentedReroll of #38 for dev revision 9bd40e4 (Sept 29, 2013)
Comment #44
jyee CreditAttribution: jyee commentedThe reroll produces this error on update.php:
Comment #45
jyee CreditAttribution: jyee commented#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.
Comment #46
jyee CreditAttribution: jyee commentedMinor tweak to use standard drupal variable names. Functionality is the same as #45.
Comment #47
DamienMcKennaThis 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.
Comment #48
HyperGlide CreditAttribution: HyperGlide commentedThanks 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)
Comment #49
HyperGlide CreditAttribution: HyperGlide commentedTest patch from no. 47.
Comment #50
DamienMcKennaThis 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.
Comment #51
DamienMcKennaThis 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.
Comment #52
HyperGlide CreditAttribution: HyperGlide commentedTested #51 and returned:
Comment #53
DamienMcKennaGah. Typo. Lets try this.
Comment #54
HyperGlide CreditAttribution: HyperGlide commentedTested #52 and returned:
Comment #55
HyperGlide CreditAttribution: HyperGlide commentedComment #56
DamienMcKennaAre you sure you're applying the patch against the lastest -dev version? Line 1100 of metatag.install is:
Comment #57
HyperGlide CreditAttribution: HyperGlide commented@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.
field_data_body
we 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.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.
Comment #58
DamienMcKennaComment #59
DamienMcKennaComment #60
DamienMcKennaComment #61
sylus CreditAttribution: sylus commentedJust 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:
Comment #62
sylus CreditAttribution: sylus commentedMy specific issue looks related to this: #2092563: Revisions & Add Entity Translation support to Meta Tags
Comment #63
HyperGlide CreditAttribution: HyperGlide commented@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.
Comment #64
sylus CreditAttribution: sylus commentedI 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?
Comment #65
HyperGlide CreditAttribution: HyperGlide commented@stylus it is marked critical b/c of #25
Comment #66
sylus CreditAttribution: sylus commentedRemoved comment as assumption was incorrect.
Comment #67
sylus CreditAttribution: sylus commentedRemoved comment as assumption was incorrect.
Comment #68
DamienMcKennaI'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.
Comment #69
DamienMcKennaBTW 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.
Comment #70
HyperGlide CreditAttribution: HyperGlide commented@DamienMcKenna thanks for the update. Any ETA on the next patch to test?
Comment #71
sylus CreditAttribution: sylus commentedJust 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:
I think it should be:
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.
Comment #72
sylus CreditAttribution: sylus commentedRemoved comment as assumption was incorrect.
Comment #73
Elijah LynnComment #74
HyperGlide CreditAttribution: HyperGlide commentedTested #72 and the same condition as reported in 57#1 above.
Comment #75
HyperGlide CreditAttribution: HyperGlide commentedComment #76
sylus CreditAttribution: sylus commentedDid 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
Comment #77
HyperGlide CreditAttribution: HyperGlide commented@sylus thanks for the update.
So are you saying that #76 is a new patch that requires testing?
Comment #78
sylus CreditAttribution: sylus commentedUpdating 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.
Comment #79
HyperGlide CreditAttribution: HyperGlide commentedTested with similar results:
Comment #80
DamienMcKennaFINALLY!!! 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.
Comment #81
DamienMcKennaSomething we need to check:
Comment #82
DamienMcKenna@HyperGlide: Can you please tell me what you see on line 1100 of metatag.install? I see the following:
For me line 1100 is the execute() line.
Comment #83
DamienMcKennaMinor adjustments to the comments to make them fit better in 80 characters.
Comment #84
DamienMcKennaWhile 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).
Comment #85
DamienMcKennaTo help with this, I've spent a good chunk of today working on this: #2152043: Integration with Devel Generate
Comment #86
DamienMcKennaThe Devel Generate integration has been committed, which will help debugging this.
Comment #87
HyperGlide CreditAttribution: HyperGlide commented@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
On 1091 I see
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.
Comment #88
jyee CreditAttribution: jyee commented#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();"
Comment #89
jyee CreditAttribution: jyee commentedAlso, 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.
Comment #90
HyperGlide CreditAttribution: HyperGlide commentedComment #91
jyee CreditAttribution: jyee commentedThe 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():
The problem is that _metatag_isdefaultrevision() has this logic:
Which means that if workbench moderation is not enabled, it will return TRUE causing metatag_entity_insert/update to return before ever saving metatags.
Comment #92
Kristen PolI 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.
Comment #93
Kristen PolI 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.
Comment #94
HyperGlide CreditAttribution: HyperGlide commented@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
Comment #95
Kristen PolThanks @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.
Comment #96
HyperGlide CreditAttribution: HyperGlide commented@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?
Comment #97
DamienMcKenna@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.
Comment #98
DamienMcKennaComment #99
DamienMcKennaThis appears to have fixed the problem of records not being deleted when an entity object is being deleted.
Comment #100
DamienMcKennaThis version loads the appropriate meta tags when viewing a node revision using the core node revision system.
Comment #101
DamienMcKennaI'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.
Comment #102
HyperGlide CreditAttribution: HyperGlide commentedI 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:
Comment #103
sylus CreditAttribution: sylus commented@HyperGlide your last comment pointed out it errored on line 1101 at this code:
This is incorrect syntax and should be:
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!
Comment #104
HyperGlide CreditAttribution: HyperGlide commented@sylus++
Running from Drush.
There where several:
Undefined property: DatabaseStatementBase::$data metatag.install:1109
But, the DB is updated!
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.
Comment #105
DamienMcKennaThanks for the fix on 103, sylus.
This is updated to only bother with the inner foreach() loop if $metatag->data exists.
Comment #107
DamienMcKenna#facepalm.
Comment #108
HyperGlide CreditAttribution: HyperGlide commentedTesting #107.
Comment #109
DamienMcKenna@Hyperglide: That's excellent! Did you verify the records were correct / workable?
Comment #110
aimeeraeHi @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!
Comment #111
DamienMcKenna@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!
Comment #112
HyperGlide CreditAttribution: HyperGlide commented@DamienMcKenna - Testing today. will revert back very soon. But so far all things are looking good. Please stand by.
Comment #113
HyperGlide CreditAttribution: HyperGlide commentedAll looks good. Agree RTBC! Roll out a beta-8 or _____? HNY!
Comment #114
DamienMcKennaThank 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
Comment #115
Elijah Lynnhttp://drupalcode.org/project/metatag.git/commit/6fbd22a
Comment #116
DamienMcKennaFYI this also resolves #2006136: Make metatag_update_7011 work with EntityTranslation.
Comment #118
eelkeblokI 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.
Comment #119
eelkeblokAs 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:
Create the following functions to create the form and have its submit handler call the update hook through batch API:
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.