Problem/Motivation

When working on #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info(), I think I found some dead code in BaseFieldDefinition. The comment responding to feedback in the original review contained in #2144327: Make all field types provide a schema(), seems to indicate it's necessary, but I can't seem to find a way to reproduce and removing it doesn't seem to break anything.

Proposed resolution

Remove the dead code.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 3014773-8.patch1.59 KBSam152
#8 interdiff.txt818 bytesSam152
#2 3014773-2.patch808 bytesSam152
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Active » Needs review
FileSize
3014773-2.patch808 bytes
joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented 
+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -734,12 +734,6 @@ public function getSchema() {
-    // some other use cases rely on identifying the first column with the key()
-    // function. Since the schema is persisted in the Field object, we take care

Are there any calls to key() in lib/Drupal/Core/Field that look like they do what this says?

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

The method returns a copy of the array, so calls to key() don't actually change the pointer of the array like the comment suggests.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Ok thanks for the explanation.

In that case, looks good to go.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

Thanks for reviewing! :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

Let's update \Drupal\field\Entity\FieldStorageConfig::getColumns() at the same time then.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs work » Needs review
FileSize
interdiff.txt818 bytes
3014773-8.patch1.59 KB

Good catch, didn't see that there.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed c76c37b and pushed to 8.7.x. Thanks!

  • catch committed c76c37b on 8.7.x 
    Issue #3014773 by Sam152, joachim, alexpott: Remove dead code in...

Status: Fixed » Closed (fixed)

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