Support from Acquia helps fund testing for Drupal Acquia logo

Comments

colan’s picture

Status: Active » Needs work
FileSize
1.33 KB

Here's the DB-altering portion; more to come.

colan’s picture

  • Added language to primary key.
  • Started changing some function signatures.
  • ET handlers are still coming.
colan’s picture

As it turns out, the function signatures didn't need to be changed.

I think I've got the display portion sorted. Feel free to begin testing it. The next big challenge will be to get the saving of the data working, both on the entity create/edit form and on the ET translation form. Things will hopefully be somewhat easier now that #1224590: Introduce entity translation CRUD hooks has been committed.

mgifford’s picture

Status: Needs work » Needs review

Changing this for the bot. Thanks for resubmitting the patch Colan.

colan’s picture

Entity-specific tags can now be added, updated, and deleted in the default language. Now onto dealing with other languages...

@mgifford: The tests should fail as this isn't done yet. If you want more confidence in the batch testing, I'd recommend helping add some tests that make it fail. ;)

plach’s picture

The new Entity Translation UI will deal with all languages automatically if you use the new entity_language() core function...

colan’s picture

@plach: As that's not guaranteed to be there, I've been doing a lot of this:
$foo = function_exists('entity_language') ? entity_language($entity_type, $entity) : $entity->language;
Please let me know if it makes sense the way I'm using it.

Here's the latest code. I had to make a bunch of minor changes (like showing the entity's language on the translation form if the translation's tags haven't been set yet.) Hopefully, everything should be in there except for the ET hooks which need implementing. I'll get to these next:

  • metatag_entity_translation_insert()
  • metatag_entity_translation_update()
  • metatag_entity_translation_delete()
colan’s picture

Got the update() one done. One down; two to go.

colan’s picture

All done. Please test and provide feedback.

I had to fix the original delete function as it wasn't actually deleting anything:

-  $ids = array_filter(array_keys($ids, 'is_numeric'));
+  $ids = array_filter($ids, 'is_numeric');
janiskam’s picture

Great job, colan! That's what I've been looking for. BTW you have to uninstall the previous metatag version, to propagate the schema changes to database (I didn't think of it at first and got an error after applying the patch).

DamienMcKenna’s picture

Seeing as this has a pretty good patch available, lets try to get this into the stable release.

colan’s picture

@janiskam: That's not necessary. Simply run update.php or use Drush.

guysaban’s picture

Hi Colan (#9),

Would you be able to provide a patch file for the latest release version of metatag module 7.x-1.0-alpha7 ?

All the best,
Guy

colan’s picture

Assigned: colan » Unassigned
Status: Needs review » Needs work

Sorry, no time right now, but it does look like it needs to be re-rolled.

mgifford’s picture

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

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

The last submitted patch, add_entity_translation_to_meta_tags-1688286-9.patch, failed testing.

guysaban’s picture

Thanks mgifford for requeueing the patch.
How do we see the test results?

mgifford’s picture

Status: Needs work » Needs review
mgifford’s picture

It said it couldn't apply, but that didn't make any sense. So I re-submitted it.

The results are available from:
http://qa.drupal.org/pifr/test/305803

Status: Needs review » Needs work

The last submitted patch, add_entity_translation_to_meta_tags-1688286-9.patch, failed testing.

guysaban’s picture

Thanks.

I looked at the log but did not find anything that helps me understand why the test failed.

Here's an excerpt from the end of the log:

[18:48:32] Command [git apply --check -p1 /var/lib/drupaltestbot/sites/default/files/review/add_entity_translation_to_meta_tags-1688286-9.patch 2>&1] failed
  Duration 0 seconds
  Directory [/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/modules/metatag]
  Status [1]
 Output: [error: patch failed: metatag.module:334
error: metatag.module: patch does not apply].
[18:48:32] Encountered error on [apply], details:
array (
  '@filename' => 'add_entity_translation_to_meta_tags-1688286-9.patch',
  '@reason' => 'Unable to apply patch.  See the log in the details link for more information',
)
mgifford’s picture

I think there's a problem with the git repository in general

$ git clone --recursive --branch master http://git.drupal.org/project/metatag.git
Cloning into 'metatag'...
remote: Counting objects: 481, done.
remote: Compressing objects: 100% (479/479), done.
remote: Total 481 (delta 301), reused 0 (delta 0)
Receiving objects: 100% (481/481), 84.59 KiB | 73 KiB/s, done.
Resolving deltas: 100% (301/301), done.
Mikes-MacBook-Pro:htdocs mike$ cd metatag
Mikes-MacBook-Pro:metatag mike$ ls
metatag.info

I just get that one file.....

alejandro_oses’s picture

FileSize
41.68 KB

Here i add the patch of colan wrapped on the final module, working.

thanks colan

Alejandro Oses
www.rootstack.com

Summit’s picture

Hi,
Will the module from #23 be integrated in metatag?
I am using latest .dev metatag of 2012-Aug-26
greetings, Martijn

guysaban’s picture

Maybe someone can try re-roll Colans patch (http://drupal.org/node/1688286#comment-6274460) for the most recent version of the metatag module or maybe for the dev version?

janiskam’s picture

FileSize
34.06 KB

I haven't created a patch, however I manually applied the changes in Colans patch to the latest Dev version of metatag. All seems fine to me but you should use the code at your own risk!

guysaban’s picture

Thanks janiskam.

I see that it does not add the additional language field (column) to the metatag table in the DB. I had to do it manually. Maybe this is stopping it from getting in to a dev release?

I rechecked. after manually building the langauge field and using the reworked module it did not work. I added content to the English field. worked. But when I added content to the Spanish entity it kept the original English content. May someone could give some input. I would be happy to check but don't know where to look.

colan’s picture

Status: Needs work » Needs review
FileSize
18.2 KB

I needed the latest dev version so I had to re-roll this.

mgifford’s picture

Issue tags: +i18n, +entities

I applied this against the 7.x-1.x git repository and got:

add_entity_translation_to_meta_tags-1688286-28.patch:78: new blank line at EOF.
+
add_entity_translation_to_meta_tags-1688286-28.patch:137: new blank line at EOF.
+
warning: 2 lines add whitespace errors.

So, I wanted to see if I could get a screenshot of this and thought I'd be able to see it here:
admin/config/search/metatags/config/node

I couldn't see it. Maybe it's because I had no data there. Anyways, just looking for better ways to test this and mark it RTBC.

Also adding i18n tag as this is that type of issue.

colan’s picture

Those warnings/errors conflict with Indenting and Whitespace which states:

All text files should end in a single newline (\n). This avoids the verbose "\ No newline at end of file" patch warning and makes patches easier to read since it's clearer what is being changed when lines are added to the end of a file.

I could take them out, but then I'd be in violation of the coding standards.

@mgifford: To test it, do the following:

  1. Go to a node's edit form, select the Meta Tags vertical tab, and then add node-specific data. Save it.
  2. Edit a translation. Update the translations for the tags you just added in the default language. Save it.
  3. Look at the page source for both languages, and you should see that the node-specific tags are properly set in each.
mgifford’s picture

FileSize
475.5 KB

Just to confirm, you want this done the old school way for translation as a node and not as a field "Enabled, with field translation"

In which case, this seems to work fine. I've attached a screenshot.

colan’s picture

I have it working with entity translation, not node translation, so I'm not sure how it would be old-school? Node translation may work as well, but I've never tested that so I have no idea if it would work.

guysaban’s picture

Hi Colan,

I used the latest dev version with your patch.

The patch went smooth as follows:

$ patch -p1 < add_entity_translation_to_meta_tags-1688286-28.patch

patching file metatag.entity_translation.inc
patching file metatag.install
patching file metatag.module
Hunk #14 succeeded at 626 (offset 12 lines).
Hunk #15 succeeded at 635 (offset 12 lines).
Hunk #16 succeeded at 642 (offset 12 lines).
Hunk #17 succeeded at 679 (offset 12 lines).
Hunk #18 succeeded at 712 (offset 12 lines).
Hunk #19 succeeded at 1067 (offset 13 lines).

I then did a db update:
drush updb

and got:

$ drush updb
WD php: PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'language' in 'field list': SELECT entity_id, data, language FROM [error]
{metatag} WHERE (entity_type = :type) AND (entity_id IN (:ids_0)); Array
(
[:type] => node
[:ids_0] => 214
)
in metatag_metatags_load_multiple() (line 350 of
website.dev/sites/all/modules/metatag/metatag.module).
Drush command terminated abnormally due to an unrecoverable error. [error]
PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'language' in 'field list': SELECT entity_id, data, language FROM {metatag} WHERE (entity_type = :type) AND (entity_id IN (:ids_0)); Array
(
[:type] => node
[:ids_0] => 214
)
in metatag_metatags_load_multiple() (line 350 of website.dev/sites/all/modules/metatag/metatag.module).
$

When I manually added the column in the DB and tested the changes did not save.
The English (default language) metatags worked fine.
When I went to the Spanish metatag entities they had the same info as the English. I tried to change and save but they kept the English info.

mkalkbrenner’s picture

Patch #28 seems to work here. But a small remaining issue is the title of the vertical tab on the content edit page which still says "Meta tags (all languages)"

plach’s picture

You need a '#multilingual' key set to TRUE in the metatag fieldset.

guysaban’s picture

Hi Plach,

Why is it necessary to add the key? what does it effect? (I am asking to learn more about this key).

All the best,
Guy

plach’s picture

It's stuff added in the latest Entity Translation -dev: http://drupalcode.org/project/entity_translation.git/blob/refs/heads/7.x....

DamienMcKenna’s picture

Has anyone tested this patch with the new beta1 of Entity Translation?

DamienMcKenna’s picture

Status: Needs review » Needs work

This needs to be rerolled against the current codebase.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
14.72 KB

Rerolled.

DamienMcKenna’s picture

FileSize
15.02 KB

This includes the '#multilingual'=>TRUE bit that was needed, per plach.

DamienMcKenna’s picture

FileSize
17.12 KB

This version has some small coding format tweaks, and replaces the module_load_include() call with a simpler include_once().

DamienMcKenna’s picture

Status: Needs review » Fixed

I tested this and it worked as advertised. Great work Colan! Committed!

guysaban’s picture

I tested this on a development site where it worked well. It is now working well on a production server.

My previous error mentioned above in #33 was due to a database problem - not a problem with the module.

Thanks to everyone that got this going, especially Colan, and to DamienMcKenna for putting a focus on it as part of his role for maintaining this module.

garrettc’s picture

Working here on quite a large multilingual development site. Great work.

DamienMcKenna’s picture

Status: Fixed » Closed (fixed)

Version 7.x-1.0-beta3 has been released and includes this work. Am closing this issue in an effort to keep the issue queue clean. Thank you all for your continued help!

lpeabody’s picture

Version: 7.x-1.x-dev » 7.x-1.0-alpha8
Status: Closed (fixed) » Active

I get the same error as in #33 when doing a `drush up` as of today. It is trying to upgrade to beta1 from alpha8.

lpeabody’s picture

Status: Active » Closed (fixed)

To get around the problem I opted to do the following instead:

drush dl metatag
# confirm the overwrite
drush updb

I was then able to run `drush up` and successfully update the rest of my modules and update the database as well.

Kristen Pol’s picture

Status: Closed (fixed) » Active

I'm using Entity Translation 7.x-1.0-beta1 and the latest Meta tags dev and ET+meta tags is not working. Is there something else that needs to be done besides running update.php? Has anyone confirmed that the latest Meta tags works with ET beta1 or later?

Kristen Pol’s picture

I have upgraded to the latest Entity Translation dev version and tested further and here's the behavior:

1) Go to French page

2) Clear the cache

3) French meta tags show up on French page

4) Switch to English page

5) French meta tags show up on English page

6) Clear the cache

7) English meta tags show up on English page

8) Switch to French page

8) English meta tags show up on French page

:/

Should this be on this issue or should I open a new issue?

DamienMcKenna’s picture

Status: Active » Closed (fixed)

Kristen: Please take a look at #1845326: Metatags not loading correctly with beta4. Thanks.

Kristen Pol’s picture

Thanks Damien... I'm following up there.

j0rd’s picture

Version: 7.x-1.0-alpha8 » 7.x-1.0-beta4

$result = db_query("SELECT entity_id, data, language FROM {metatag} WHERE (entity_type = :type) AND (entity_id IN (:ids))", array(
     ':type' => $type,
     ':ids' => $ids,
));

Because of this query, metatag is not longer query'ing over MySQL indexes. At least according to my MySQL slow log.

To shut MySQL up, I've had to add these indexes.

ALTER TABLE metatag ADD index(entity_id);
ALTER TABLE metatag ADD index(entity_type);

I noticed because I have a site with like 40,000 nodes and started doing a bulk delete and mysql slow logs went nuts.

DamienMcKenna’s picture

@j0rd: Please open a new issue rather than posting unrelated comments on an issue that was closed almost three months ago. Thanks.

Adam Clarey’s picture

I'm having a big problem with this module on PHP 5.4.

in metatag.module at line 76 there is the code:

$hooks_entity_translation = array(
'entity_translation_insert',
'entity_translation_update',
'entity_translation_delete',
);
$hooks_entity_translation = array_fill_keys($hooks_entity_translation, array('group' => 'entity_translation'));

To add the above data to the 'hook_info' array generated in module_hook_info. The problem is that the entity translation module is also adding these hooks to the array meaning in hook info you see:

{entity_translation_insert :
{group :
0 : 'entity_translation',
1 : 'entity_translation'
}
}

Instead of being just:

{entity_translation_insert :
{group : 'entity_translation'}
}

Which it needs to be. The problem occurs later in module_implements() - includes/module.inc at line 709:

$include_file = isset($hook_info[$hook]['group']) && module_load_include('inc', $module, $module . '.' . $hook_info[$hook]['group']);

Because $hook_info[$hook]['group'] is now an array and not a string, it throws out a long list of errors:

Notice: Array to string conversion in module_implements() (line 709 of ~/includes/module.inc).

My question is, is the above code in metatag.module actually needed if it is being added by the entity_translation module? And if so, then there needs to be a way around this problem.

Adam

DamienMcKenna’s picture

@rabbit_media: Please don't post unrelated questions on a closed issue, instead please open a new issue. Thanks.

DamienMcKenna’s picture

Issue summary: View changes

Added link to ET's project page.