Follow-up of #1497374: Switch from Field-based storage to Entity-based storage. those constants should move to where they are used, this is currently causing trouble in #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API.

CommentFileSizeAuthor
#14 load-constants-2079011-14.patch20.34 KBalexpott
#14 diff-8-14.txt739 bytesalexpott
#8 6-8-interdiff.txt1.15 KBalexpott
#8 load-constants-2079011-8.patch20.26 KBalexpott
#6 load-constants-2079011-6.patch19.81 KBBerdir
#6 load-constants-2079011-6-interdiff.txt4.4 KBBerdir
#3 load-constants-2079011-3.patch15.48 KBBerdir
#3 load-constants-2079011-3-interdiff.txt1.68 KBBerdir
#1 load-constants-2079011-1.patch14.81 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
load-constants-2079011-1.patch14.81 KB

There's one instance where I'm not sure what it is, see @todo.

Also weird, the storage uses this constant only internally (except field stuff), but EFQ uses it publicly, with a different meaning (load the revision vs query all the revisions). I think it would be better to create separate constants here.

Status: Needs review » Needs work

The last submitted patch, load-constants-2079011-1.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
load-constants-2079011-3-interdiff.txt1.68 KB
load-constants-2079011-3.patch15.48 KB

Missed two instances too aggressive with deleting...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: +API change

Tagging.

Status: Needs review » Needs work

The last submitted patch, load-constants-2079011-3.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
load-constants-2079011-6-interdiff.txt4.4 KB
load-constants-2079011-6.patch19.81 KB

More costant fixes.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Moved field.module constants related to storage to EntityStorageControllerInterface » Move field.module constants related to storage to EntityStorageControllerInterface
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
load-constants-2079011-8.patch20.26 KB
6-8-interdiff.txt1.15 KB 
+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
@@ -202,6 +202,7 @@ public function query($use_groupby = FALSE) {
+      // @todo: What is this?
       $rkey = $this->definition['is revision'] ? 'FIELD_LOAD_REVISION' : 'FIELD_LOAD_CURRENT';

We can just use the constants properly here

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs review » Reviewed & tested by the community

Looks good now.

Status: Reviewed & tested by the community » Needs work
Issue tags: -API change

The last submitted patch, load-constants-2079011-8.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
Issue tags: +API change

#8: load-constants-2079011-8.patch queued for re-testing.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs review » Reviewed & tested by the community

Looks still ok :)

yched’s picture

yched CreditAttribution: yched commented

Thanks folks ! I'm fine with this getting in to clean obvious scope issues, but I opened #2081513: Deprecate FIELD_LOAD_* constants for more thoughts on those constants.

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
@@ -202,7 +203,7 @@ public function query($use_groupby = FALSE) {
       $fields = array();
-      $rkey = $this->definition['is revision'] ? 'FIELD_LOAD_REVISION' : 'FIELD_LOAD_CURRENT';
+      $rkey = $this->definition['is revision'] ? EntityStorageControllerInterface::FIELD_LOAD_REVISION : EntityStorageControllerInterface::FIELD_LOAD_CURRENT;
       // Go through the list and determine the actual column name from field api.

Side note: this code looks wrong / broken anyway, see #2081533: Views "group by" on "Field API" fields broken

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
diff-8-14.txt739 bytes
load-constants-2079011-14.patch20.34 KB

Patch no longer applies :) rerolled - I'm all for getting this is and then working out if we can do without them

The interdiff was empty :) so attaching diff that shows a line that's unchanged by this patch has been changed.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed/pushed to 8.x, thanks!

Needs a change notice.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Title: Move field.module constants related to storage to EntityStorageControllerInterface » Change notice: Move field.module constants related to storage to EntityStorageControllerInterface
Issue tags: +Approved API change
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Change notice: Move field.module constants related to storage to EntityStorageControllerInterface » Move field.module constants related to storage to EntityStorageControllerInterface
Priority: Critical » Normal
Status: Active » Fixed

Change notice created: https://drupal.org/node/2082357

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: -Needs change record

Untagging. Please remove the tag when the change notification task is completed.