Updated: Comment #32

Problem/Motivation

The support for foreign keys in the field schema is nearly completely broken, and only works by chance:

  • The processing done in _field_sql_storage_schema() uses an undefined variable name
  • The foreign keys are not updated when the field is updated by field_update_field()

We have some tests, but they pass by chance.

Proposed resolution

  • Fix the brokenness
  • Extend the tests

Remaining tasks

A patch has been created and applied to 8.x, and a patch exists and is RTBC for 7.x. Last remaining item is to commit to 7.x.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#29 field-foreign_keys-1416506-29-D7.patch6.92 KByched
PASSED: [[SimpleTest]]: [MySQL] 40,324 pass(es).
[ View ]
#29 interdiff.txt807 bytesyched
#27 field-foreign_keys-1416506-27-D7.patch6.1 KByched
FAILED: [[SimpleTest]]: [MySQL] 40,337 pass(es), 4 fail(s), and 3 exception(s).
[ View ]
#27 interdiff.txt1.34 KByched
#25 field-foreign_keys-1416506-25-D7.patch6.05 KByched
FAILED: [[SimpleTest]]: [MySQL] 40,331 pass(es), 4 fail(s), and 3 exception(s).
[ View ]
#22 field-foreign_keys-1416506-21.patch6.3 KByched
PASSED: [[SimpleTest]]: [MySQL] 54,865 pass(es).
[ View ]
#22 interdiff.txt1.41 KByched
#20 field-foreign_keys-1416506-20.patch6.31 KByched
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#20 interdiff.txt3.37 KByched
#19 1416506-19.patch5.99 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 54,499 pass(es).
[ View ]
#19 interdiff.txt488 bytesswentel
#17 1416506-17.patch5.99 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 54,456 pass(es), 7 fail(s), and 6 exception(s).
[ View ]
#17 interdiff.txt1.52 KBswentel
#15 1416506-15.patch5.64 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 54,446 pass(es), 0 fail(s), and 12 exception(s).
[ View ]
#6 1416506-field-foreign-keys.patch6.56 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1416506-field-foreign-keys_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 1416506-field-foreign-keys-7.patch6.4 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] 36,949 pass(es), 4 fail(s), and 212 exception(es).
[ View ]
#2 1416506-field-foreign-keys.patch6.5 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] 33,298 pass(es), 2 fail(s), and 217 exception(es).
[ View ]
#1 1416506-field-foreign-keys-test.patch4.73 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] 33,354 pass(es), 3 fail(s), and 29 exception(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new4.73 KB
FAILED: [[SimpleTest]]: [MySQL] 33,354 pass(es), 3 fail(s), and 29 exception(es).
[ View ]

Test only patch.

StatusFileSize
new6.5 KB
FAILED: [[SimpleTest]]: [MySQL] 33,298 pass(es), 2 fail(s), and 217 exception(es).
[ View ]

Test+fix.

Version:8.x-dev» 7.x-dev
StatusFileSize
new6.4 KB
FAILED: [[SimpleTest]]: [MySQL] 36,949 pass(es), 4 fail(s), and 212 exception(es).
[ View ]

Drupal 7 version of the same.

Version:7.x-dev» 8.x-dev

Status:Needs review» Needs work

The last submitted patch, 1416506-field-foreign-keys-7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1416506-field-foreign-keys_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fix for the missing key in field_update_field().

Status:Needs review» Needs work

The last submitted patch, 1416506-field-foreign-keys.patch, failed testing.

Status:Needs work» Needs review

#6: 1416506-field-foreign-keys.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1416506-field-foreign-keys.patch, failed testing.

Issue tags:+needs backport to D7

#1969048: hook_field_schema() cannot vary based on field settings is a related issue (and possible duplicate).

Priority:Normal» Major

Bumping to major. This affects several contribs and blocks a regression from moving entityreference into core: #1847582: Create a foreign key to the target entity type base type in Entity-reference.

Issue tags:+Field API

Will need rerolling now CMI conversion is in. Tagging as well.

Assigned:Unassigned» swentel

Will reroll later this evening

Assigned:swentel» Unassigned

Ok, this is trickier after the config conversion, will look later unless someone beats me to it.

Status:Needs work» Needs review
StatusFileSize
new5.64 KB
FAILED: [[SimpleTest]]: [MySQL] 54,446 pass(es), 0 fail(s), and 12 exception(s).
[ View ]

- edit -

Sorry for no interdiff, had todo drupal_get_schema('table', TRUE) as we're in a unit test.

Status:Needs review» Needs work

The last submitted patch, 1416506-15.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.52 KB
new5.99 KB
FAILED: [[SimpleTest]]: [MySQL] 54,456 pass(es), 7 fail(s), and 6 exception(s).
[ View ]

Fixing tests. I guess Damien or Yched can sign this off now - if ok of course :)

Status:Needs review» Needs work

The last submitted patch, 1416506-17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new488 bytes
new5.99 KB
PASSED: [[SimpleTest]]: [MySQL] 54,499 pass(es).
[ View ]

Duh.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new3.37 KB
new6.31 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Looks good.

Just minor adjustments:
- Fixed stale code comments
- Removed t()s around test messages

Status:Reviewed & tested by the community» Needs work

The last submitted patch, field-foreign_keys-1416506-20.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.41 KB
new6.3 KB
PASSED: [[SimpleTest]]: [MySQL] 54,865 pass(es).
[ View ]

Trailing whitespace.
+ needless double quotes.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Version:8.x-dev» 7.x-dev
Status:Fixed» Patch (to be ported)

Status:Patch (to be ported)» Needs review
StatusFileSize
new6.05 KB
FAILED: [[SimpleTest]]: [MySQL] 40,331 pass(es), 4 fail(s), and 3 exception(s).
[ View ]

Backport.

Status:Needs review» Needs work

The last submitted patch, field-foreign_keys-1416506-25-D7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.34 KB
new6.1 KB
FAILED: [[SimpleTest]]: [MySQL] 40,337 pass(es), 4 fail(s), and 3 exception(s).
[ View ]

Hm, is that better ?

Status:Needs review» Needs work

The last submitted patch, field-foreign_keys-1416506-27-D7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new807 bytes
new6.92 KB
PASSED: [[SimpleTest]]: [MySQL] 40,324 pass(es).
[ View ]

Right, pre-Field/CMI this requires a fix in field_update_field(), and @Damz's original patches accounted for that.

Scratch #27, interdiff is against #25.

This is not a proper review, but I patched 7.22 with the 4 non-test-related lines in that patch (#29). It appears to have fixed the problem in #1340748: Add CTools relationship.

I'm not sure if it's worse to hack 7.22 in my case or if I should rather run 7.x-dev + patch as I need this pretty much right now.

Status:Needs review» Reviewed & tested by the community

+++ b/modules/field/modules/field_sql_storage/field_sql_storage.moduleundefined
@@ -188,7 +188,7 @@ function _field_sql_storage_schema($field) {
-    foreach ($specification['columns'] as $column => $referenced) {
+    foreach ($specification['columns'] as $column_name => $referenced) {
       $sql_storage_column = _field_sql_storage_columnname($field['field_name'], $column_name);
       $current['foreign keys'][$real_name]['columns'][$sql_storage_column] = $referenced;

awesome!

This worked for me in combination with #44 from #1340748: Add CTools relationship. After applying these patches, I was able to add a Taxonomy from Node relationship in Panels. That wasn't showing as an option before for that field, because it was using an entity reference instead of a term reference.

This helps a lot. Thanks for working on it!

Status:Reviewed & tested by the community» Fixed
Issue tags:+7.23 release notes

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

Issue summary:View changes

Updated issue summary.