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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
4.73 KB

Test only patch.

Damien Tournoud’s picture

Test+fix.

Damien Tournoud’s picture

Version: 8.x-dev » 7.x-dev
FileSize
6.4 KB

Drupal 7 version of the same.

Damien Tournoud’s picture

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

Status: Needs review » Needs work

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

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
6.56 KB

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.

andros’s picture

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.

David_Rothstein’s picture

Issue tags: +Needs backport to D7

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

Dave Reid’s picture

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.

swentel’s picture

Issue tags: +Field API

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

swentel’s picture

Assigned: Unassigned » swentel

Will reroll later this evening

swentel’s picture

Assigned: swentel » Unassigned

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

swentel’s picture

Status: Needs work » Needs review
FileSize
5.64 KB

- 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.

swentel’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
5.99 KB

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
488 bytes
5.99 KB

Duh.

yched’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.37 KB
6.31 KB

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.

yched’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.41 KB
6.3 KB

Trailing whitespace.
+ needless double quotes.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Dave Reid’s picture

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

Status: Patch (to be ported) » Needs review
FileSize
6.05 KB

Backport.

Status: Needs review » Needs work

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

yched’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
6.1 KB

Hm, is that better ?

Status: Needs review » Needs work

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

yched’s picture

Status: Needs work » Needs review
FileSize
807 bytes
6.92 KB

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.

malberts’s picture

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.

andypost’s picture

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!

mixxmac’s picture

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!

David_Rothstein’s picture

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

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.