From #1735118: Convert Field API to CMI. Not really sure how to explain this one, so I'll just quote what we said on-issue.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -0,0 +1,463 @@
+      if (array_intersect(array_keys($schema['columns']), field_reserved_columns())) {

@xjm:

It looks like field_reserved_columns() returns only 'deleted'. A procedural wrapper to return that seems odd; followup?

@swentel:

Not for now, this function is also used in _field_sql_storage_columnname() for instance which does not get a $field object

@yched:

Yeah, I actually have no clue what the logic around field_reserved_columns() is. This was introduced by the EFQv2 patch, and the issue has no explanations about that.

But yeah, I tried to see if it could be moved to a method on Field.php, but the consuming code won't allow that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

chx’s picture

As far as entity_query() is concerned, ->condition('field_name.deleted', 0) needs to be recognized as a valid condition and this is the way it recognizes it. Field.php refuses creating those columns to avoid confusion.

swentel’s picture

Right, makes sense. Having a procedural wrapper is not that bad, the function explains what it does and it's better than hardcoding it. So as far as I'm concerned, we can just close this one.

yched’s picture

I'd tend to inline / hardcode that "list of forbidden column names" (currently restricted to 1 item...) directly in the Field.php class...
I don't really see the value of delegating this to an external functional helper now ?

swentel’s picture

@yched static method then ? e.g. _field_sql_storage_columnname does not get a field object ..

swentel’s picture

Status: Active » Needs review
FileSize
3.46 KB

Let's move this to a static method.

yched’s picture

Status: Needs review » Needs work
+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -446,7 +446,7 @@ public function getSchema() {
+      if (array_intersect(array_keys($schema['columns']), $this->field_reserved_columns())) {

I think the recommended practice is static::method().

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -563,5 +563,13 @@ public function unserialize($serialized) {
+  public static function field_reserved_columns() {

Method name needs to get CamelCased :-)

amateescu’s picture

I think the name of the method should be getReservedColumns().

swentel’s picture

Status: Needs work » Needs review
FileSize
2.47 KB
3.45 KB
chx’s picture

I will let xjm RTBC this; the patch is harmless I just see no point.

chx’s picture

Assigned: Unassigned » xjm
xjm’s picture

Assigned: xjm » Unassigned
Status: Needs review » Reviewed & tested by the community

This seems much more sane.

xjm’s picture

Title: Figure out what the deal is with field_reserved_columns() » Move field_reserved_columns() to a static method

Better title. ;)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.