Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

Status: Active » Needs review
FileSize
1.95 KB

This adds a hook_update_N() script to migrate the data, which is also executed during the new hook_install(). If there's a lot of data this could take a while. It also is set to append " | [site:name]" to the value, which should provide the expected result.

Note: this does not do anything for any per-entity or per-bundle settings, right now it just converts the {page_title} database records.

DamienMcKenna’s picture

Title: Write upgrade path from Page_Title to Metatags » Upgrade path: Page_Title
Issue tags: +Upgrade path
DamienMcKenna’s picture

Component: Miscellaneous » Code
DamienMcKenna’s picture

Status: Needs review » Needs work

Needs further work.

HyperGlide’s picture

Exciting to see a migration path for other modules to help bring them all together under "meta tag"

Related to #1282806: Upgrade path: Meta tags quick

Thank you!

DamienMcKenna++

iamEAP’s picture

Related to the issue referenced by HyperGlide in #5, I wonder if it'd be more appropriate to make a separate upgrade/import module (perhaps with drush support) rather than have everything in a series of upgrade hooks.

As it is, installing or updating the module will perform the data migration without asking. Seems like it should be an explicit action.

jenlampton’s picture

I'm getting a message after updating metatag to 7.x-1.0-beta4 that I should disable the page title module, but I don't want to loose it's data. Is the best path forward to apply this patch, run the update, and then disable & uninstall page_title?

jenlampton’s picture

Updating to beta4 + patch hosed my metatag data & page_title data, now all nodes say they are using defaults when there is clearly data in the database for metatags, but the key on language seems to be confusing matters.

Going to try beta3 instead.

jenlampton’s picture

Status: Needs review » Needs work

Things don't look much better after updating to beta3, there's still a key in the 'data' record for language, and all my nodes still think they are using defaults.

Going to try beta2 instead.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
2.02 KB

Okay, that worked great! I think the problems in beta3 and beta4 are unrelated to this patch.

An Update to Beta2 + attached patch migrated in all the data from page_title. It's rerolled here with a new update number (7003) and removal of a duplicate dsm, but will need another re-roll after the language / translation issues are sorted.

The last submitted patch, metatag-upgrade_path_from_page_title-1658970-10.patch, failed testing.

DamienMcKenna’s picture

DamienMcKenna’s picture

@jenlampton: The beta3 and beta4 problems are because of #1845326: Metatags not loading correctly with beta4, which has just been committed, so the -dev version should work much more reliably.

HyperGlide’s picture

Status: Needs work » Needs review
Issue tags: -Upgrade path

Status: Needs review » Needs work
Issue tags: +Upgrade path

The last submitted patch, metatag-upgrade_path_from_page_title-1658970-10.patch, failed testing.

HyperGlide’s picture

Was not able to apply #10 against beta7 or dev.

It is trying to patch /sites/all/modules/contrib/metatag/metatag.install however in our D 7.22 our modules are within /sites/all/modules/metatag/metatag.install.

Is this a bug?

HyperGlide’s picture

Status: Needs work » Needs review
FileSize
2.16 KB

Attempt to update the patch to the latest dev.

HyperGlide’s picture

Status: Needs review » Needs work

Applied patch cleanly to dev on test site.

running update.php following errors stopped the update.

An AJAX HTTP error occurred. 
HTTP Result Code: 500 Debugging information follows. 
Path: http///meta.dev.domain.com/update.php?op=selection&token=0yysOh2aIQ&id=3158&op=do 
StatusText: error 
ResponseText: Recoverable fatal error: Argument 3 passed to metatag_metatags_load_multiple() must be an array, none given, 
called in /sites/all/modules/metatag/metatag.install on line 934 
and defined in metatag_metatags_load_multiple() (line 351 of /sites/all/modules/metatag/metatag.module).
DamienMcKenna’s picture

I've decided that all import/migration scripts will be done as drush commands, so this definitely needs work.

HyperGlide’s picture

Talking to Cottser in IRC he express that

in the dev version metatag_metatags_load() calls metatag_metatags_load_multiple() without the third argument, so there may be a bug in the dev version that is causing the error you saw.

http://drupalcode.org/project/metatag.git/blob/refs/heads/7.x-1.x:/metat...

subhojit777’s picture

Patch in #17 applied successfully and executed drush updb but it is giving loads of:

Missing argument 4 for metatag_metatags_save(), called in /var/www/hcl/docroot/sites/all/modules/contrib/metatag/metatag.install on line 906 and defined metatag.module:385

Also it is destroying the existing metatags after update.

HyperGlide’s picture

@subhojit777 -- did you read the comments after #17?

subhojit777’s picture

@HyperGlide but I am not using the dev version. I am using 7.x-1.0-beta7 version of metatag.

I am using this module for migrating nodewords to metatag, after that I am doing drush updb and I see that existing metatag settings for nodes like description and keywords are no longer maintained.

I will try doing drush updb first then upgrade from nodewords to metatag.

subhojit777’s picture

Status: Needs work » Needs review
FileSize
4.39 KB

The patch given #17 does not migrates pagetitle to metatags following the metatag config settings. For example, if a content type has certain metatag title setting then title should appear as per the setting, I think this should be the expected behavior. Here is a new patch that does this job.

subhojit777’s picture

The patch given above does not trims whitespaces. Forgot to do that :)
Here is the correct patch.

HyperGlide’s picture

I believe #25 needs to have the code updated as a result of other recent comments. Any interest to help get this accomplished and I can help support the testing.

barryvdh’s picture

I tried the patch with the latest release and with the current dev, but I get the following error:
Fatal error: Class name must be a valid object or a string in in /var/www/html/aosmith/public/includes/common.inc, line 7837

Any ideas?

HyperGlide’s picture

stefan.r’s picture

Status: Needs work » Needs review
FileSize
4.51 KB

Anyone still have this same error?

stefan.r’s picture

That was merely a re-roll, here's that same patch with some rework and testing.

Status: Needs review » Needs work
jeffdiecks’s picture

Issue tags: +Needs reroll
stefan.r’s picture

Status: Needs work » Needs review
FileSize
4.77 KB

re-roll

Status: Needs review » Needs work
mrjmd’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2015
FileSize
4.77 KB

Rerolled against dev.

DamienMcKenna’s picture

Issue tags: -Needs reroll
DamienMcKenna’s picture

mrconnerton’s picture

The patch in #38 didn't apply to my fresh dev checkout of the module but I manually applied the patch and seems to work fine. I ran the update, disabled page_title module, and then cleared the cache. After checking a few nodes everything looks good.

I would have prefered the update completely set the page title tag instead of just replacing [current-page:title]. For example my "default" was [current-page:title] | [site:name] so now all my manually set page titles have | MySiteName on the end.

Other than that, looks good!

mrconnerton’s picture

Actually I have found I have to save the node for the new meta tag to take affect. I'm not sure if this is caching or what. I've done full cache clears and the old title was appearing until I did a quick edit/save on the node.

DamienMcKenna’s picture

Status: Needs review » Needs work

@mrconnerton: thanks for the review, sounds like this needs some further work.

mrconnerton’s picture

Also I'm new to working with metatag code but I believe the first key in meta tag data is suppose to be the lang code: http://cgit.drupalcode.org/metatag/tree/metatag.module#n525

When I ran the update it didn't have that so the first key in the tables is "value" with "title" as the langcode.

DamienMcKenna’s picture

Component: Code » Importing/migrating data
DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
4.21 KB

Rerolled.

stefan.r’s picture

page_title can also be used as a views filter, I don't think metatag has such functionality, does it?

DamienMcKenna’s picture

FileSize
4.29 KB

Fixed a problem with the handling of metatag_metatags_load().

@stefan.r: I wasn't aware of that. Could you please open a new issue for that and describe how it works? Thanks!

Alex-CDE’s picture

Hi,
I tried the patch with the latest release #46, but I get the following error:
Fatal error: Class name must be a valid object or a string in /home/***/includes/common.inc on line 7963

DamienMcKenna’s picture

@Alex-CDE: Can you please install the Devel module and in its settings page set the error handler to one of the Krumo ones, then see if you can work out what class name it's failing on? Thanks.

DamienMcKenna’s picture

FileSize
7.31 KB

This converts the update script above to a Drush script.

DamienMcKenna’s picture

Status: Needs review » Fixed

Committed! Thanks all!

  • DamienMcKenna committed 9138316 on 7.x-1.x
    Issue #1658970 by DamienMcKenna, stefan.r, subhojit777, HyperGlide,...
DamienMcKenna’s picture

FYI this has been released in the new v7.x-1.7 and I've spun off a new issue to handle the additional Page Title settings: #2539444: Finish converting all Page Title settings

Status: Fixed » Closed (fixed)

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

Benia’s picture

I tried to run the patch with git after situating the patch file in the Metatag module folder but the patch failed saying some files where not present (when they were indeed present).