Similar to #1965208: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem, this will finally allow us to write generic code for "* reference" fields by using a unified name (target_id) for the reference column.

CommentFileSizeAuthor
#49 1996714-49.patch57.97 KBamateescu
#45 1996714-45.patch58.93 KBamateescu
#45 interdiff.txt458 bytesamateescu
#42 1996714-42.patch58.93 KBamateescu
#42 interdiff.txt3.75 KBamateescu
#40 1996714-40.patch55.18 KBamateescu
#38 1996714-38.patch58.93 KBamateescu
#38 interdiff.txt3.98 KBamateescu
#36 1996714-36.patch56.15 KBamateescu
#36 interdiff.txt3.41 KBamateescu
#34 1996714-34.patch54.93 KBamateescu
#31 1996714-31.patch55.92 KBamateescu
#27 1996714-27.patch55.45 KBamateescu
#27 interdiff.txt2.25 KBamateescu
#25 1996714-25.patch53.4 KBamateescu
#21 1996714-21.patch80.76 KBamateescu
#20 1996714-20.patch55.86 KBamateescu
#20 interdiff.txt3.84 KBamateescu
#18 1996714-18.patch58.59 KBamateescu
#3 1996714-3.patch58.66 KBamateescu
#1 1996714.patch57.87 KBamateescu

Comments

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 1996714.patch57.87 KB

Let's see how this one fares with the testbot.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Issue tags: +Media Initiative
amateescu’s picture

amateescu commented
StatusFileSize
new 1996714-3.patch58.66 KB

Rerolled for #1620010: Move LANGUAGE constants to the Language class.

amateescu’s picture

amateescu commented
Issue tags: -sprint, -Media Initiative, -Entity Field API

#3: 1996714-3.patch queued for re-testing.

fago’s picture

fago
Primary language German
Location Vienna
commented
Issue tags: +sprint, +Media Initiative, +Entity Field API

#3: 1996714-3.patch queued for re-testing.

fago’s picture

fago
Primary language German
Location Vienna
commented

Patch looks great to me already. Do we have test coverage on this update functions?

Status: Needs review » Needs work
Issue tags: -sprint, -Media Initiative, -Entity Field API

The last submitted patch, 1996714-3.patch, failed testing.

das-peter’s picture

das-peter commented
Status: Needs work » Needs review
Issue tags: +sprint, +Media Initiative, +Entity Field API

#3: 1996714-3.patch queued for re-testing.

amateescu’s picture

amateescu commented

Do we have test coverage on this update functions?

Yep, this patch is tested by UserPictureUpgradePathTest.

index c85f5e4..cca5f55 100644
--- a/core/modules/system/lib/Drupal/system/Tests/Upgrade/UserPictureUpgradePathTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UserPictureUpgradePathTest.php
@@ -69,7 +69,7 @@ public function testUserPictureUpgrade() {
     // Check the user picture and file usage record.
     $user = user_load(1);
-    $file = file_load($user->user_picture[Language::LANGCODE_NOT_SPECIFIED][0]['fid']);
+    $file = file_load($user->user_picture[Language::LANGCODE_NOT_SPECIFIED][0]['target_id']);
     $this->assertEqual('public://user_pictures_dir/faked_image.png', $file->uri);
     $usage = file_usage()->listUsage($file);
     $this->assertEqual(1, $usage['file']['user'][1]);
yched’s picture

yched commented

Just like I posted in #1965208: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem,

No need to wait for it, but when #1969728: Implement Field API "field types" as TypedData Plugins lands, this change will in practice mean "the (here) 'file field type' plugin extends the "entity reference field type' plugin", which makes very much sense.

In other words, +1

In this specific case, though, I think we'll want "image field" to extend "file field" (that's what image.module's current implementations of hook_field_*() imply - just defer to the equivalent functions for file field).
Not sure whether it makes sense / is possible to go there right now, or whether this specific point should wait for #1969728: Implement Field API "field types" as TypedData Plugins, but in the end we'll most probably have ImageItem extends FileItem.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The reason I didn't do that initially is that's not completely true for the field item properties. Images don't have the display/description columns so I thought it would be a bit weird to extend but then remove those, at least in the current state of those classes, where they only define that and not much more.

But it will probably make more sense once these classes do more than just define the properties.

yched’s picture

yched commented

Makes sense, works for me.

amateescu’s picture

amateescu commented

In this specific case, though, I think we'll want "image field" to extend "file field" ...

Yeah, I tried that in #1839066-10: Implement the new entity field API for the image field type, got overruled, didn't really care enough to argue further.

So.. everyone seems to like this patch, but not enough to rtbc it? :)

yched’s picture

yched commented

Sorry, don't count on me for reviews / RTBCs this week, very limited bandwidth :-(

swentel’s picture

swentel commented

#3: 1996714-3.patch queued for re-testing.

swentel’s picture

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

This is very straight forward, +1 from me too. RTBC when it comes back green.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1996714-3.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 1996714-18.patch58.59 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 1996714-18.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.84 KB
new 1996714-20.patch55.86 KB

I've no idea what #2001310: Disallow firing hooks during update changed so the image upgrade path doesn't work anymore, but since image depends on file, we can just do this instead.

amateescu’s picture

amateescu commented
StatusFileSize
new 1996714-21.patch80.76 KB

This patch has been massacrated by #1818568: Convert files to the new Entity Field API, now I know why Berdir didn't want to rtbc it sooner :) YAR!

Status: Needs review » Needs work
Issue tags: -sprint, -Media Initiative, -Entity Field API

The last submitted patch, 1996714-21.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

#21: 1996714-21.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +sprint, +Media Initiative, +Entity Field API

The last submitted patch, 1996714-21.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 1996714-25.patch53.4 KB

Rerolled for #2020677: file_field_prepare_view() should not delete items and not including the patch from #1965208: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem this time.

Status: Needs review » Needs work

The last submitted patch, 1996714-25.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.25 KB
new 1996714-27.patch55.45 KB
effulgentsia’s picture

effulgentsia commented
Priority: Normal » Major

For similar reason as #1965208-52: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem, raising this to major.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: -sprint, -Media Initiative, -Entity Field API

#27: 1996714-27.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +sprint, +Media Initiative, +Entity Field API

The last submitted patch, 1996714-27.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 1996714-31.patch55.92 KB
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: -sprint, -Media Initiative, -Entity Field API

#31: 1996714-31.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +sprint, +Media Initiative, +Entity Field API

The last submitted patch, 1996714-31.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 1996714-34.patch54.93 KB

Status: Needs review » Needs work

The last submitted patch, 1996714-34.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.41 KB
new 1996714-36.patch56.15 KB

Rerolled after #1969728: Implement Field API "field types" as TypedData Plugins and fixed those tests.

Status: Needs review » Needs work

The last submitted patch, 1996714-36.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.98 KB
new 1996714-38.patch58.93 KB

This should fix the upgrade path :) Also found some more references to 'fid' that needed to be replaced.

Status: Needs review » Needs work

The last submitted patch, 1996714-38.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 1996714-40.patch55.18 KB

Reroll hell!

Status: Needs review » Needs work

The last submitted patch, 1996714-40.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.75 KB
new 1996714-42.patch58.93 KB

That was probably a merge conflict.. fixed :)

effulgentsia’s picture

effulgentsia commented 
+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php
@@ -93,7 +93,7 @@ public function getPropertyDefinitions() {
-    $definition = \Drupal::typedData()->getDefinition('field_item:' . $field->type);
+    $definition = \Drupal::service('plugin.manager.entity.field.field_type')->getDefinition($field->type);

This looks fine, but why is it needed by this patch?

+++ b/core/modules/file/file.install
@@ -288,3 +296,46 @@ function file_update_8002() {
+          'not null' => FALSE,

Earlier in the patch, file_field_schema() defines this as TRUE.

effulgentsia’s picture

effulgentsia commented

The feedback in #43 is really minor. Once it's addressed/answered, please feel free to RTBC without waiting for me (since I'm 8 hours behind Dublin time).

amateescu’s picture

amateescu commented
StatusFileSize
new interdiff.txt458 bytes
new 1996714-45.patch58.93 KB

Good catch in the schema, fixed :)

About the change in getPropertyDefinitions(), that's because of the failures from #36. Basically, the typed data manager invokes a hook (data_type_info) which is now forbidded in update functions.

Status: Needs review » Needs work
Issue tags: -sprint, -Media Initiative, -Entity Field API

The last submitted patch, 1996714-45.patch, failed testing.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
Issue tags: +sprint, +Media Initiative, +Entity Field API

#45: 1996714-45.patch queued for re-testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

I'm very sorry, but this needs a re-roll because of a stupid comment fix in Tables.php, as that has been moved.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 1996714-49.patch57.97 KB

Sure thing ;)

effulgentsia’s picture

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

Awesome.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: -sprint

Removing sprint tag.

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

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
Issue tags: -Media Initiative +D8Media

Swap to the right media tag.