Split from #2846614: Incorrect field name is used in views integration for multi-value base fields.

Problem/Motivation

From \Drupal\views\EntityViewsData::mapFieldDefinition().

// @todo Introduce concept of the "main" column for a field, rather than
//   assuming the first one is the main column. See also what the
//   mapSingleFieldViewsData() method does with $first.

When building the entity views data, the class identifies the main property/column of a field as the first column from field schema. This has been designed early when we had no "main property" API in place.

Proposed resolution

Fix the @todo by using \Drupal\Core\Field\FieldStorageDefinitionInterface::getMainPropertyName().

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#31 2847034-nr-bot.txt132 bytesneeds-review-queue-bot
#18 2847034-18.patch5.78 KBMunavijayalakshmi
#17 interdiff-13-to-17.txt1.14 KBclaudiu.cristea
#17 2847034-17.patch5.8 KBclaudiu.cristea
#13 interdiff-10-to-12.txt2.87 KBclaudiu.cristea
#13 2847034-12.patch5.79 KBclaudiu.cristea
#10 2847034-10-TEST-ONLY.patch3.25 KBclaudiu.cristea
#10 2847034-10.patch6.88 KBclaudiu.cristea
#6 interdiff-2-to-6.txt2.64 KBclaudiu.cristea
#6 2847034-6.patch3.92 KBclaudiu.cristea
#2 2847034-2.patch3.65 KBclaudiu.cristea

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Active » Needs review
StatusFileSize
new 2847034-2.patch3.65 KB

Patch.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
Related issues: +#2846614: Incorrect field name is used in views integration for multi-value base fields
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Looks good to me, not sure if the argument change in mapSingleFieldViewsData() is considered an API change, though. I would tend to say "yes", so maybe we just have to deprecate the parameter? Not sure though.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

@tstoeckler, according to https://www.drupal.org/core/d8-bc-policy#protected-methods

Protected methods of a class should be assumed @internal and subject to change unless either the class or method itself are marked with @api

It's not a BC break. Let's RTBC it if otherwise looks OK for you and see what committers are saying,

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 2847034-6.patch3.92 KB
new interdiff-2-to-6.txt2.64 KB

OK, here's a version that doesn't change the signature, except that is renaming the local variable $first to $is_main_property but this is not a BC break. Also, the value passed there is the same as in HEAD meaning that there's no risk if somebody has extended mapSingleFieldViewsData().

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

dawehner
Primary language German
commented

I'm wondering whether there should be a test with multiple fields, where the main property is not the first available one?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Also note that it is possible that a field type has no main property, what happens then? Not sure what the current state is, but DER for example doesn't really have a main property, neither entity_id nor entity_type make sense on their own, and what I discussed with @jibran is that it should return NULL.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 2847034-10.patch6.88 KB
new 2847034-10-TEST-ONLY.patch3.25 KB

Also note that it is possible that a field type has no main property, what happens then? Not sure what the current state is, but DER for example doesn't really have a main property, neither entity_id nor entity_type make sense on their own, and what I discussed with @jibran is that it should return NULL.

@Berdir, In this use case, we need ::getMainPropertyName() only to append or not a suffix to the views field column title. Looking at the patch code, when ::getMainPropertyName() returns NULL, all columns will get the suffix. And I think this what we want.

@dawehner, added the test. The test-only patch is interdiff.

The last submitted patch, 10: 2847034-10.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

It is true that conceptually DER shouldn't have the main property but to support Entity field rendering for views and EFQ we started using target_id as main property in #2818261: Add target_id as the main property just like ERItem.
In core, if you want to test multiple properties then shape field type can be used.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 2847034-12.patch5.79 KB
new interdiff-10-to-12.txt2.87 KB

Revert unneeded changes. Fix docs.

dawehner’s picture

dawehner
Primary language German
commented

It would be great to get feedback from @jibran / @berdir whether they believe the NULL prefix solves it. Personally I would cast NULL explicit to an empty string at least though.

+++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
@@ -517,6 +526,10 @@ public function testBaseTableFields() {
+    // Check that minor properties have the ' (<property>)' suffix.

I'd have used "Check that all other properties ..." it just reads nicer for some reason :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Once the patch is ready I can test it against https://github.com/jibran/dynamic_entity_reference. DER has a bunch of tests related to views data.

The last submitted patch, 10: 2847034-10-TEST-ONLY.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 2847034-17.patch5.8 KB
new interdiff-13-to-17.txt1.14 KB

It would be great to get feedback from @jibran / @berdir whether they believe the NULL prefix solves it.

@dawehner, Thank you for review. I don't understand exactly about what prefix are you talking. I think the code is clear:

+++ b/core/modules/views/src/EntityViewsData.php
@@ -366,19 +366,15 @@ protected function mapFieldDefinition($table, $field_name, FieldDefinitionInterf
+    $main_property = $this->getFieldStorageDefinitions()[$field_name]->getMainPropertyName();
...
     foreach ($field_column_mapping as $field_column_name => $schema_field_name) {
...
+      $is_main_property = $field_column_name === $main_property;

If $main_property is NULL, then $is_main_property will evaluate to FALSE (note, I used === strict comparison). In this case all field's column titles will be suffixed with " ({column})". But isn't this the expected behavior when no column is the main column?

I added the docs fix. I changed the placeholder representation to use the curly brackets and used the term "column" which seems more appropriate in the context of field schema. (IMO this should be subject of a future field type terminology consolidation: 'column' vs. 'property').

@jibran It would be nice if you can run a test with this patch.

Munavijayalakshmi’s picture

Munavijayalakshmi commented
StatusFileSize
new 2847034-18.patch5.78 KB

Rerolled the patch.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

joachim commented
Issue tags: +Needs subsystem maintainer review

Going by #17, it looks like this needs approval from a maintainer rather than a general review.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 2847034-nr-bot.txt132 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.