| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | field system |
| Category: | bug report |
| Priority: | major |
| Assigned: | catch |
| Status: | closed (fixed) |
| Issue tags: | D8MI, langcode, language-content, translatable fields |
Issue Summary
Problem/Motivation
As per #1216094: We call too many things 'language', clean that up we are converting schemas and parallel APIs to use 'langcode' instead of 'language' where a language code is involved. The use of 'language' inconsistently for language objects and language codes in Drupal 7 keeps throwing people off the cliff and makes understanding the API much harder.
This issue is about field data storage needing the same conversion to move from language to langcode. Will need updates and upgrade tests but first of all it would be a well focused search and replace wherever field language is involved. The tricky part is that field language maps to generated schema pieces in the database for example, so is used in schemas under all kinds of dynamic names.
Proposed resolution
Rename all occurences of language into langcode
Remaining tasks
Notify devel : #1292294: Respect field translatability when generating content, otherwise it gets lost
API changes
Use 'langcode' instead of 'language' for field queries.
Original report by Gábor Hojtsy
As per #1216094: We call too many things 'language', clean that up we are converting schemas and parallel APIs to use 'langcode' instead of 'language' where a language code is involved. The use of 'language' inconsistently for language objects and language codes in Drupal 7 keeps throwing people off the cliff and makes understanding the API much harder.
This issue is about field data storage needing the same conversion to move from language to langcode. Will need updates and upgrade tests but first of all it would be a well focused search and replace wherever field language is involved. The tricky part is that field language maps to generated schema pieces in the database for example, so is used in schemas under all kinds of dynamic names.
Comments
#1
You can use
drush php-eval "print_r(drupal_get_complete_schema());"to check for language.
http://api.drupal.org/api/drupal/core!includes!bootstrap.inc/function/drupal_get_complete_schema/8
Using patch drush: #1396178: Add properties output as a --format and
drush php-eval "print drush_output_format_properties(drupal_get_complete_schema());" | grep language\.typegives a nice list
comment.fields.language.type = varchar
field_data_comment_body.fields.language.type = varchar
field_revision_comment_body.fields.language.type = varchar
field_data_body.fields.language.type = varchar
field_revision_body.fields.language.type = varchar
field_data_field_tags.fields.language.type = varchar
field_revision_field_tags.fields.language.type = varchar
field_data_field_image.fields.language.type = varchar
field_revision_field_image.fields.language.type = varchar
node.fields.language.type = varchar
date_format_locale.fields.language.type = varchar
users.fields.language.type = varchar
#2
#3
I start working on alpha order
drush php-eval "print drush_output_format_properties(drupal_get_complete_schema(TRUE));" | grep language\.type | grep field_ | sort
field_data_body.fields.language.type = varchar
field_data_comment_body.fields.language.type = varchar
field_data_field_image.fields.language.type = varchar
field_data_field_tags.fields.language.type = varchar
field_revision_body.fields.language.type = varchar
field_revision_comment_body.fields.language.type = varchar
field_revision_field_image.fields.language.type = varchar
field_revision_field_tags.fields.language.type = varchar
which leaves out
comment.fields.language.type = varchardate_format_locale.fields.language.type = varchar
locales_target.fields.language.type = varchar
node.fields.language.type = varchar
users.fields.language.type = varchar
which are covered by other issues.
#4
First stab @ field_sql_storage , field.multilingual and locale.module. Let's see how much dies.
#5
Looks good, just one minor comment:
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.moduleundefined@@ -143,13 +143,13 @@ function _field_sql_storage_schema($field) {
+ 'description' => 'The langcode for this data item.',
Let's say "language code" when in 'docs', not langcode :)
#6
Fixed the text from #5
Another stab ... changed all(?) code occurences
'language' and ->languagefor field and field_uiThere are a lot of langcode related strings and function names still left for change.
[edit]
Following modules use field alteration of some kind
grep -rl "_field_.*_alter" * | cut -c 1-120core/modules/field/field.api.php
core/modules/field/field.attach.inc
core/modules/field/field.module
core/modules/field/field.multilingual.inc
core/modules/field/modules/list/list.module
core/modules/field/modules/options/options.module
core/modules/field/tests/field.test
core/modules/field/tests/field_test.module
core/modules/field/tests/field_test.storage.inc
core/modules/field_ui/field_ui.test
core/modules/locale/locale.module
core/modules/node/node.module
core/modules/node/node.test
core/modules/poll/poll.module
core/modules/rdf/rdf.module
core/modules/taxonomy/taxonomy.module
core/modules/user/user.module
#7
#8
The last submitted patch, field-language-rename-1439692-6.patch, failed testing.
#9
Failure of TextTranslationTestCase was my bad ... that test case is test a node/add/article
Failure of LanguageUpgradePathTestCase needs some work as the column is changed from language to langcode.
Same goes for FilledUpgradePathTestCase
Failure of EntityFieldQueryTestCase was my bad ...
I expect now only failure of both upgrade path test.
Next step is to http://drupal.org/node/1429136 (UpgradePath test cases)
#10
The last submitted patch, field-language-rename-1439692-9.patch, failed testing.
#11
Hmmm ... EntityFieldQueryTestCase still fails :(
One down ... three tests to fix.
#12
I failed so solve EntityFieldQueryTestCase completely (6 out of 8 are solved)
drush test-run EntityFieldQueryTestCaseEntity query 116 passes, 2 fails, and 0 exceptions
Test EntityFieldQueryTestCase->testEntityFieldQueryMetaConditions() failed: Test with a grouped language meta condition (empty result set).
Test EntityFieldQueryTestCase->testEntityFieldQueryMetaConditions() failed: Test with a grouped delta + language meta condition (empty result set, language condition unsatisifed).
Checking the documentation for these test give me no clue :(
We still need a
<?phpfunction field_update_8000() {
?>
#13
I'm puzzled how to alter the schema for this unknown amount of fields having a language column. A code example would be welcome ;)
This needs API Change record for contrib modules to use langcode instead of language.
#14
The last submitted patch, field-language-rename-1439692-12.patch, failed testing.
#15
I think we can do that upgrade path for the mysql field storage engine only. Afaik amitai came up with some working code for that over at #1319040: Remove "target_type" column from db (d7) in entity reference, which could serve as guidance. I don't think we should rely upon field_info_fields() though, but better use low-level functions for retrieving info about existing fields.
#16
@fago tnx
What about other storage engine? That sounds like a D8 release blocker is it?
Not using field_info_fields was what I thought too.
The link #1319040: Remove "target_type" column from db patch from #37 was useful.
I guess we could use code like field_read_fields without invoking modules.
I give it a try this week(end).
#17
I added a
<?phpfunction field_sql_storage_update_8000(&$sandbox) {
?>
@fago
I'm still concerned with "We only upgrade mysql". Please elaborate on that.
Let's see what the test bot says.
#18
The last submitted patch, field-language-rename-1439692-17.patch, failed testing.
#19
Nice ... compared to #12 we have 2 exceptions down.
The failures are in
Entity query (EntityFieldQueryTestCase) [Entity API] 138 2 0
Message Group Filename Line Function Status
Test with a grouped language meta condition (empty result set). Other entity_query.test 1165 EntityFieldQueryTestCase->testEntityFieldQueryMetaConditions()
Test with a grouped delta + language meta condition (empty result set, language condition unsatisifed). Other entity_query.test 1192 EntityFieldQueryTestCase->testEntityFieldQueryMetaConditions()
which reads garble to me. The code that is :(
So some guides needed still.
#20
These tests needs way more info.
<?php$query = new EntityFieldQuery();
$query
->entityCondition('entity_type', 'test_entity', '=')
->fieldCondition($this->fields[0], 'value', 0, '=', NULL, 'group')
->fieldLanguageCondition($this->fields[0], LANGUAGE_NONE, '<>', NULL, 'group');
$this->assertEntityFieldQuery($query, array(), t('Test with a grouped language meta condition (empty result set).'));
?>
and
<?php$query = new EntityFieldQuery();
$query
->entityCondition('entity_type', 'test_entity', '=')
->fieldCondition($this->fields[0], 'value', 0, '=', 'delta', 'language')
->fieldDeltaCondition($this->fields[0], 1, '<', 'delta', 'language')
->fieldLanguageCondition($this->fields[0], LANGUAGE_NONE, '<>', 'delta', 'language');
$this->assertEntityFieldQuery($query, array(), t('Test with a grouped delta + language meta condition (empty result set, language condition unsatisifed).'));
?>
#21
I reverted the language_group arguments back to 'language'
Added extra debug info on assertEntityFieldQuery failure.
The 2 failures are still here so this needs help :(
#22
The last submitted patch, field-language-rename-1439692-21.patch, failed testing.
#23
#21: field-language-rename-1439692-21.patch queued for re-testing.
#24
@chx pointed out http://api.drupal.org/api/drupal/modules%21field%21modules%21field_sql_s... which loops through the conditions, and it looks up 'language_group' by concatenating the foreach value before '_group'. Since you renamed that to 'langcode' but not the key itself, all you got was a missing key, so no good results. If we rename 'langugae_group' to 'langcode_group' too, we get those tests pass again, since they will find their values :)
Attached the interdiff as well. For EFQ to work properly, the first part of the last line change was important. The others I've renamed for consistency.
#25
@clemens.tolboom:
AFAIK we support other storage engines only on the API level, we don't account for them in update functions as it would be nearly impossible to avoid hook invocations, which in this context are BAD!
+++ b/core/modules/field/field.api.php
@@ -1421,7 +1421,7 @@ function hook_field_language_alter(&$display_language, $context) {
+ locale_field_language_fallback($display_language, $context['entity'], $context['langcode']);
+++ b/core/modules/field/field.attach.inc
@@ -1174,7 +1174,7 @@ function field_attach_view($entity_type, $entity, $view_mode, $langcode = NULL)
+ $options = array('langcode' => $display_language);
+++ b/core/modules/field/tests/field.test
@@ -2790,7 +2790,7 @@ class FieldTranslationsTestCase extends FieldTestCase {
+ $options['langcode'][$id] = field_language($entity_type, $entity, NULL, $display_language);
...
@@ -2810,7 +2810,7 @@ class FieldTranslationsTestCase extends FieldTestCase {
@@ -2810,7 +2810,7 @@ class FieldTranslationsTestCase extends FieldTestCase {
$grouped_results = _field_invoke_multiple('test_op_multiple', $entity_type, $entities, $null, $null, $options);
foreach ($grouped_results as $id => $results) {
foreach ($results as $langcode => $result) {
- $this->assertTrue(isset($options['language'][$id]), t('The result language %language for entity %id was correctly suggested (display language: %display_language).', array('%id' => $id, '%language' => $langcode, '%display_language' => $display_language)));
+ $this->assertTrue(isset($options['langcode'][$id]), t('The result language %language for entity %id was correctly suggested (display language: %display_language).', array('%id' => $id, '%language' => $langcode, '%display_language' => $display_language)));
+++ b/core/modules/field/tests/field_test.module
@@ -116,7 +116,7 @@ function field_test_field_available_languages_alter(&$languages, $context) {
+ locale_field_language_fallback($display_language, $context['entity'], $context['langcode']);
+++ b/core/modules/locale/locale.module
@@ -325,7 +325,7 @@ function locale_field_language_alter(&$display_language, $context) {
+ locale_field_language_fallback($display_language, $context['entity'], $context['langcode']);
Actually also the display language is a display language code :)
+++ b/core/modules/field/field.api.php
@@ -1732,20 +1732,20 @@ function hook_field_storage_write($entity_type, $entity, $op, $fields) {
+ ->condition('langcode', $languages, 'IN')
...
+ ->condition('langcode', $languages, 'IN')
+++ b/core/modules/field/field.attach.inc
@@ -202,7 +202,7 @@ function _field_invoke($op, $entity_type, $entity, &$a = NULL, &$b = NULL, $opti
+ $languages = _field_language_suggestion($available_languages, $options['langcode'], $field_name);
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.module
@@ -395,20 +395,20 @@ function field_sql_storage_field_storage_write($entity_type, $entity, $op, $fiel
+ ->condition('langcode', $languages, 'IN')
...
+ ->condition('langcode', $languages, 'IN')
+++ b/core/modules/field/tests/field_test.storage.inc
@@ -136,7 +136,7 @@ function field_test_field_storage_write($entity_type, $entity, $op, $fields) {
+ if ($row->type == $entity_type && $row->entity_id == $id && in_array($row->langcode, $languages)) {
I guess
$languagesshould become$langcodes.+++ b/core/modules/field/field.attach.inc@@ -319,7 +319,7 @@ function _field_invoke_multiple($op, $entity_type, $entities, &$a = NULL, &$b =
+ $language = !empty($options['langcode'][$id]) ? $options['langcode'][$id] : $options['langcode'];
$languageshould be$langcode. In this light$available_languagesshould become$available_langcodes.+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install@@ -77,3 +77,36 @@ function _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
+ $fields = field_read_fields(array(), array('include_deleted' => TRUE, 'include_inactive' => TRUE));
I think we want
_update_7000_field_read_fields()here to avoid hook invocations.+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.module@@ -623,8 +623,8 @@ function _field_sql_storage_query_field_conditions(EntityFieldQuery $query, Sele
+ // Add delta / langcode group conditions.
"langcode conditions": do we really want to go this far?
26 days to next Drupal core point release.
#26
I've applied the suggestion from @plach except the hook_update_8000 as that lies outside my comfort zone right now.
<?phpfunction _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
?>
I also changed text by changing language when langcode is meant afaik. This needs a review.
See inter-diff.
#27
#28
+++ b/core/modules/field/field.api.php
@@ -1406,10 +1406,10 @@ function hook_field_attach_prepare_translation_alter(&$entity, $context) {
+ * This hook is invoked to alter the array of display language codes for the given
+++ b/core/modules/field/field.multilingual.inc
@@ -84,10 +84,10 @@ function field_language_delete() {
+ * language codes will be returned, otherwise only LANGUAGE_NONE will be returned.
@@ -103,36 +103,36 @@ function field_language_delete() {
+ * Process the given language code suggestion based on the available language codes.
@@ -227,9 +227,9 @@ function field_has_translation_handler($entity_type, $handler = NULL) {
+ * Otherwise, it returns the current, global language code; or the site's default
@@ -249,15 +249,15 @@ function field_valid_language($langcode, $default = TRUE) {
+ * Returns the display language code for the fields attached to the given entity.
...
+ * requested language code and the actual data available in the fields themselves.
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.test
@@ -95,15 +95,15 @@ class FieldSqlStorageTestCase extends DrupalWebTestCase {
+ // Add a translation in an unavailable language code and verify it is not loaded.
$eid = $evid = 1;
- $unavailable_language = 'xx';
Comments do not wrap at column 80.
+++ b/core/modules/field/field.attach.inc@@ -316,12 +316,12 @@ function _field_invoke_multiple($op, $entity_type, $entities, &$a = NULL, &$b =
+ $available_langcodes = field_available_languagecodes($entity_type, $field);
This should be
field_available_languages(). I'm wondering how the hell is possible for this code not to fail tests.+++ b/core/modules/field/field.multilingual.inc@@ -103,36 +103,36 @@ function field_language_delete() {
+ $drupal_static_fast['field_languagecodes'] = &drupal_static(__FUNCTION__);
Here and below
field_languagecodesshould befield_langcodes.+++ b/core/modules/field/field.multilingual.inc@@ -142,28 +142,28 @@ function field_available_languages($entity_type, $field) {
+ // If we have a language code suggestion and the suggested language code
+ // is available, we return only it.
'is' shoud go one line up: comments should wrap as close as possible to column 80.
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install@@ -77,3 +77,36 @@ function _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
+ $fields = field_read_fields(array(), array('include_deleted' => TRUE, 'include_inactive' => TRUE));
As I wrote previously
field_read_fieldsshould be safely replaceable with a_update_7000_field_read_fields.+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install@@ -77,3 +77,36 @@ function _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
+ // Drop indexes
...
+ // Alter field
...
+ // Reinstall indexes
Missing trailing dots.
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.module
@@ -386,39 +386,39 @@ function field_sql_storage_field_storage_write($entity_type, $entity, $op, $fiel
+ $all_languagecodes = field_available_languages($entity_type, $field);
+ $field_languagecodes = array_intersect($all_languagecodes, array_keys((array) $entity->$field_name));
...
+ $langcodes = !empty($entity->$field_name) ? $field_languagecodes : $all_languagecodes;
...
+ foreach ($field_languagecodes as $langcode) {
+++ b/core/modules/field/tests/field_test.storage.inc
@@ -126,17 +126,17 @@ function field_test_field_storage_write($entity_type, $entity, $op, $fields) {
+ $all_languagecodes = field_available_languages($entity_type, $field);
+ $field_languagecodes = array_intersect($all_languagecodes, array_keys((array) $entity->$field_name));
@@ -150,7 +150,7 @@ function field_test_field_storage_write($entity_type, $entity, $op, $fields) {
+ foreach ($field_languagecodes as $langcode) {
languagecodes -> langcodes
+++ b/core/modules/field/tests/field_test.storage.inc@@ -126,17 +126,17 @@ function field_test_field_storage_write($entity_type, $entity, $op, $fields) {
+ $languages = !empty($entity->$field_name) ? $field_languagecodes : $all_languagecodes;
...
+ if ($row->type == $entity_type && $row->entity_id == $id && in_array($row->langcode, $languages)) {
$languages -> $langcodes
$all_languagecodes -> $all_langcodes
+++ b/core/modules/locale/locale.module@@ -340,23 +340,23 @@ function locale_field_language_alter(&$display_language, $context) {
+ foreach ($display_langcode as $field_name => $field_language) {
field_langcode
+++ b/core/modules/locale/locale.module@@ -365,7 +365,7 @@ function locale_field_language_fallback(&$display_language, $entity, $langcode)
foreach ($fallback_candidates as $fallback_language) {
if (isset($entity->{$field_name}[$fallback_language])) {
- $display_language[$field_name] = $fallback_language;
+ $display_langcode[$field_name] = $fallback_language;
break;
fallback_language -> fallback_langcode
21 days to next Drupal core point release.
#29
This should be field_available_languages(). I'm wondering how the hell is possible for this code not to fail tests.This is indeed interesting. There seems to be a problem, maybe the tests are not completed.
Here is a new patch which reroles the patch and fixes the issues from above.
#30
The last submitted patch, field-language-rename-1439692-29.patch, failed testing.
#31
There is another patch for the testbot.
#32
Reviewing...
#33
Open points still stainding according franskuipers and clemens.tolboom
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install
@@ -77,3 +77,36 @@ function _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
We should delete this function.
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install@@ -77,3 +77,36 @@ function _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
+ $fields = _update_7000_field_read_fields();
This is a Drupal 7 function which we should not use.
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install@@ -77,3 +77,36 @@ function _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
+ drupal_load('module', 'field_sql_storage');
Is this ok to use?
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install@@ -77,3 +77,36 @@ function _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
+ $schema += _field_sql_storage_schema($field);
Is this hook-less?
#34
To test the upgrade path I extended the node type "Article" to have two instances (single & multivalue) of every core field type and generated 50 nodes with content.
The only issue I cam across was the fact that the scheme wasn't rebuild after the update and thus the site had a WSDO until a full cache flush was done.
Adding a
drupal_get_schema(NULL, TRUE);after the schema update solves this issue.#35
We (FransKuipers and me) rewrote field_sql_storage_update_8000 which is now without hook invocation.
Please review :)
<?php
function field_sql_storage_update_8000(&$sandbox) {
if (db_table_exists('field_config')) {
$result = db_select('field_config', 'fc')
->fields('fc', array('field_name'))
->condition('storage_type', 'field_sql_storage')
->execute()
;
$primary_key = array (
'entity_type',
'entity_id',
'deleted',
'delta',
'langcode',
);
$langcode_index = array(
'langcode',
);
$field_langcode = array(
'type' => 'varchar',
'length' => 32,
'not null' => true,
'default' => '',
);
foreach ($result as $row) {
$field_name = $row->field_name;
$tables = array(
'field_data_' . $field_name,
'field_revision_' . $field_name,
);
foreach ($tables as $table) {
db_drop_primary_key($table);
db_drop_index($table, 'language');
db_change_field($table, 'language', 'langcode', $field_langcode);
db_add_primary_key($table, $primary_key);
db_add_index($table, 'langcode', $langcode_index);
}
}
}
?>
We commented out
<?php// Force a schema rebuild.
//drupal_get_schema(NULL, TRUE);
?>
as it's unclear to us _AND_ it involves invoking hook_schema.
#36
The last submitted patch, field-language-rename-1439692-35.patch, failed testing.
#37
We fixed the _revision primary key.
#38
Tested the patch again and this time the explicit rebuild of the schema cache wasn't necessary :)
Thus I removed the lines I added earlier.
#39
+++ b/core/modules/entity/tests/entity_query.testundefined@@ -1526,7 +1526,9 @@ class EntityFieldQueryTestCase extends DrupalWebTestCase {
+ if (!$this->assertEqual($results, $intended_results, $message)) {
+ debug($results, $message);
Just a leftover debug statement
+++ b/core/modules/field/field.multilingual.incundefined@@ -104,36 +104,37 @@ function field_language_delete() {
+ drupal_alter('field_available_languages', $langcodes, $context);
+++ b/core/modules/field/field.multilingual.incundefined@@ -281,38 +284,38 @@ function field_valid_language($langcode, $default = TRUE) {
+ drupal_alter('field_language', $display_langcode, $context);
I'm wondering whether it's intended to rename the hooks and some other functions as well.
Just from the perspective of a module developer i would though preper the current version.
I read the full big patch and didn't saw any problem with the renaming of language to langcode.
#40
@dawehner good catch for the debug.
But are we changing the hooks? We only changed the hook parameter names from language* to langcode* right? Or do I miss something? One could argument the hook names should be changed too.
Thanks for the checks :)
#41
I don't think we want to change also hooks and function names, but we should hear Gabor's voice on this.
#42
I definitely replied in IRC that I think it should not be part of this issue, if ever :) We should debate that elsewhere definitely. This is about the schema and its update, lets try to focus on that.
#43
+++ b/core/modules/field/field.multilingual.inc@@ -104,36 +104,37 @@ function field_language_delete() {
- drupal_alter('field_available_languages', $languages, $context);
- $field_languages[$entity_type][$field_name] = $languages;
+ drupal_alter('field_available_languages', $langcodes, $context);
+++ b/core/modules/field/field.multilingual.inc@@ -281,38 +284,38 @@ function field_valid_language($langcode, $default = TRUE) {
- drupal_alter('field_language', $display_language, $context);
+ drupal_alter('field_language', $display_langcode, $context);
Attention: Only the name of the variables, used for hook invocation, are changed. There's no such thing as a renamed hook.
+++ b/core/modules/entity/tests/entity_query.testundefined@@ -1526,7 +1526,9 @@ class EntityFieldQueryTestCase extends DrupalWebTestCase {
+ if (!$this->assertEqual($results, $intended_results, $message)) {
+ debug($results, $message);
As this debug call is conditional it might make even sense to keep it (at least it doesn't seem to harm a thing), but I don't know what the rules for such constructs are.
#44
@das-peter: let's keep the debug then.
@Gábor Hojtsy: Who must review this to get this RTBC?
#45
@Clemens: I don't have a specific list! @fubhy, @plach, @dawhenever, @Clemens (yes), etc. were very great reviewers so far, many people worked on this patch, so cross-reviewing with them seems like would be great in itself. It is not about new architecture or shiny new stuff, it is about DX improvements, so I think its fine if we cross-review.
#46
@plach could you please review the last hurdle mentioned by you in #25 + #28 and resolved similar to #35 in patch #37 concerning
<?phpfunction field_sql_storage_update_8000(&$sandbox) {
?>
I think #38 is then RTBC :)
(as we keep the debug and don't change hooks #39 - #44 )
#47
I fixed some language occurence left. I also rewrote the update function to use the (supposedly?) proper update API function. We really need @catch to have a look to it sooner or later. Maybe we should assign this to him when this is RTBC (and now IMO it is).
+++ b/core/modules/field/field.attach.inc@@ -316,12 +316,12 @@ function _field_invoke_multiple($op, $entity_type, $entities, &$a = NULL, &$b =
+ $language = !empty($options['langcode'][$id]) ? $options['langcode'][$id] : $options['langcode'];
+ $langcodes = _field_language_suggestion($available_langcodes, $language, $field_name);
$language => $langcode
+++ b/core/modules/field/field.multilingual.inc@@ -104,36 +104,37 @@ function field_language_delete() {
* @param $language_suggestion
$langcode_suggestion
+++ b/core/modules/field/field.multilingual.inc
@@ -143,28 +144,28 @@ function field_available_languages($entity_type, $field) {
+ // If we have a language suggestion and the suggested language is
+ // available, we return only it.
@@ -229,9 +230,9 @@ function field_has_translation_handler($entity_type, $handler = NULL) {
+ * Otherwise, it returns the current, global language code; or the site's default
Wrong comment wrapping.
+++ b/core/modules/field/tests/field.test@@ -2935,48 +2935,48 @@ class FieldTranslationsTestCase extends FieldTestCase {
+ $requested_langcodes = $enabled_langcodes[0];
+ $display_langcodes = field_language($entity_type, $entity, NULL, $requested_langcodes);
Here and below $requested_langcodes should be singular, as only a single language is requested.
+++ b/core/modules/locale/locale.module@@ -365,32 +365,32 @@ function locale_field_language_alter(&$display_language, $context) {
+ * @param $field_langcode
If we want to make this change, this should be plural here and below.
-4 days to next Drupal core point release.
#48
The last submitted patch, field-language-rename-1439692-47.patch, failed testing.
#49
Weird, I manually tested the upgrade path and it completed nicely...
#50
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install@@ -77,3 +77,51 @@ function _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
+ db_add_primary_key($table, $primary_key_data);
Hmm, here is the culprit :)
Attached a working patch plus interdiff with #38.
-4 days to next Drupal core point release.
#51
Gábor Hojtsy:
Since the only part that still needs review is the update function and no one here seems to be comfortable with reviewing it, what about marking this RTBC and assigning it to @catch so he can provide us any feedback about possible pending concerns?
#52
Looks like the question is down to whether we want _field_sql_storage_revision_tablename() duplicated for the update and whether _update_7000_field_read_fields() should be renamed?
#53
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install@@ -77,3 +77,52 @@ function _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
+ // Retrieve field data.
+ $fields = _update_7000_field_read_fields(array('storage_type' => 'field_sql_storage'));
+
This function (name) may never be used for D8. AFAIK it is a helper function for D6 to D7 upgrades.
That's why I used the db_query.
<?phpif (db_table_exists('field_config')) {
$result = db_select('field_config', 'fc')
->fields('fc', array('field_name'))
->condition('storage_type', 'field_sql_storage')
->execute()
;
?>
+++ b/core/modules/field/field.multilingual.inc@@ -152,8 +152,8 @@ function _field_language_suggestion($available_langcodes, $langcode_suggestion,
+ // If we have a language suggestion and the suggested language is available,
+ // we return only it.
Should this be langcode as well?
'If we have a langcode suggestion and the suggested langcode is available,'
+++ b/core/modules/field/tests/field.test@@ -2945,8 +2945,8 @@ class FieldTranslationsTestCase extends FieldTestCase {
- $requested_langcodes = $enabled_langcodes[0];
- $display_langcodes = field_language($entity_type, $entity, NULL, $requested_langcodes);
+ $requested_langcode = $enabled_langcodes[0];
+ $display_langcodes = field_language($entity_type, $entity, NULL, $requested_langcode);
Good catch over the plural form.
I guess $display_langcodes should now also singular instead of plural.
#54
#55
@clemens: yes, that is why I'm saying if we want to use the _update_7000_field_sql_storage_write() function in the D8 upgrade, we'd want to rename it maybe to _update_8000_field_sql_storage_write() or somesuch?
#56
@Gábor Hojtsy: He ... we past each others comment :)
It could be good to transform that function but should we do it for this issue? I somehow guess yes.
If we do that it needs a test as I think its data column contains values also available through the individual columns. Not 100% sure.
<?phpfunction _update_7000_field_read_fields(array $conditions = array(), $key = 'id') {
...
foreach ($query->execute() as $record) {
$field = unserialize($record['data']);
...
?>
#57
Fixed the language suggestion comment and another 'language' occurrence.
I recall from the deadly #1164852: Inconsistencies in field language handling that it's absolutely discouraged to directly query
field_config, since we cannot rely on it being always there. I really think using _update_7000_field_read_fields()is the right thing to do here, since it encapsulates the field retrieval logic, and no, I don't think renaming it from 7000 to 8000 belongs to an issue dealing with language code renamings. My point is we should really ask someone knowledgeable, as @catch.#58
@plach: as you put it that way we certainly have to use a function like that :)
+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.install@@ -77,3 +77,52 @@ function _update_7000_field_sql_storage_write($entity_type, $bundle, $entity_id,
+ $fields = _update_7000_field_read_fields(array('storage_type' => 'field_sql_storage'));
So we agree this function should be reused by renaming it to _update_8000_field_read_fields or maybe a better name as @Gábor Hojtsy suggested too.
What about adding hookless into it's name and make it public. It is now used only by hook_update_8000 but is that it's only use case?
It is similar to http://api.drupal.org/api/drupal/core!modules!field!field.crud.inc/function/field_read_fields/8 right?
<?phpfunction field_read_fields_hookless()
function _field_read_fields_hookless()
?>
#59
Sounds as good plan but for the life of me I don't get why we have to discuss it here, however the update prefix means exactly hookless. I don't think we need a new terminology.
#60
@plach : You just want to use the function whatever it's name. Sounds like a brilliant plan :) ... or I completely misunderstood.
@Gábor Hojtsy guess we should follow along @plach suggestion from #51 and mark this as RTBC and assign to catch.
#61
(OT)
@clemens.tolboom:
Trying to explain myself better: having a better name for that function, making clear it's an API function pertaining the upgrade path sounds like a good plan. I don't think the hookless terminology is something we need to introduce since we already have the "update_*" prefix.
#62
_update_7000_field_read_fields() would be easy to rename, it is not invoked ever in Drupal 8 as far as my grep shows. I think it was merely missed from being removed when the D6 -> D7 update functions were removed. I'd say that it is the task of patches introducing update path use of functions to make sure those are available properly provided as pure update functions. Therefore I'd at least rename this function to _update_8000_....().
Then there is _field_sql_storage_revision_tablename() which is a 6 line function with a very simple condition. Whether we should keep using that is a good question really. Safest is to duplicate that too as an update function. That would not hurt. Either way, once the _update_7000_...() function is renamed, I'm comfortable marking this RTBC in any case. I can easily predict @catch would ask for the 7000 rename :)
#63
There is a bunch of _7000 functions in field.install, not mentioning another one in node.install and one in field_sql_storage.install. None of them is used, should we rename them all for consistency?
#64
@plach: I think *that* could be for another issue. Patches are responsible for their own update stuff :)
#65
Here it is.
#66
I think that looks good to go now.
#67
Committed to 8.x. Thanks!
#68
@catch:
Would you please confirm
field_sql_storage_update_8000()is ok?#69
No it's not quite right, there's no reason to do this since the schema for field config hasn't changed, those functions are tied to schema version, not API version.
-function _update_7000_field_read_fields(array $conditions = array(), $key = 'id') {+function _update_8000_field_read_fields(array $conditions = array(), $key = 'id') {
See also #1134088: Properly document update-api functions.
#70
@catch so we shouldn't have renamed the _update_7000_field_read_fields ?
Leave it 'as is' as the schema we are querying is still a D7 schema? Makes sense.
So all _7000_ functions are meant to work on a D7 scheme in a D8 context.
(I've read and commented on #1134088: Properly document update-api functions)
#71
@catch: all the update functions, quite a few of which work with the Drupal 7 schema state are named _800* too. The previous examples in #1134088: Properly document update-api functions, before your latest example today at http://drupal.org/node/1134088#comment-5849018 showed update helper functions in the _8000 space used in update functions in the 8000 space. Your latest example makes the differentiation better, and makes it clear that we should name this back. Thanks for that.
#72
@ Gábor Hojtsy: yes anything named 8000 that's not specifically related to a Drupal 8 schema change is named incorrectly, that's discussed in #1239370: Module updates of new modules in core are not executed which is another pile of evil :(
I've committed/pushed the follow-up, thanks!
#73
While attempting to write a change notice node for this, I found that we updated the schema handling in _update_7000_field_sql_storage_write(), but did not rename that function. As per the above discussion, that should have been renamed. Verified that with @catch in IRC. This function is not invoked anywhere, so there is no use to keep a D7 specific version around. Later update functions would invoke the D8 version (and set an update function dependency on field_sql_storage_update_8000().
Verified that this is not invoked via http://api.drupal.org/api/drupal/core%21modules%21field%21modules%21fiel... and looking for invocations in the committed patch too.
#74
OK, committed that one too.
#75
Ok, wrote up an initial version of a change notice node at http://drupal.org/node/1525236
#76
Removing sprint tag too.
#77
Automatically closed -- issue fixed for 2 weeks with no activity.