CommentFileSizeAuthor
#121 field_type_clean-2015697-121.patch808 bytessmiletrl
#117 typed-data-file-image-2015697-117.patch59 KByched
#117 interdiff.txt1.21 KByched
#115 typed-data-file-image-2015697-115.patch58.96 KByched
#115 interdiff.txt856 bytesyched
#112 typed-data-file-image-2015697-112.patch58.98 KByched
#112 interdiff.txt924 bytesyched
#110 typed-data-file-image-2015697-110.patch58.98 KByched
#110 interdiff.txt925 bytesyched
#108 typed-data-file-image-2015697-108.patch58.98 KByched
#108 interdiff.txt2.41 KByched
#103 typed-data-file-image-2015697-103.patch60.8 KBclaudiu.cristea
#103 interdiff.txt7.7 KBclaudiu.cristea
#96 interdiff.txt1.89 KBclaudiu.cristea
#96 typed-data-file-image-2015697-95.patch61.64 KBclaudiu.cristea
#92 reference_fields.png21.96 KBBerdir
#90 typed-data-file-image-2015697-90.patch59.99 KBclaudiu.cristea
#90 interdiff.txt15.91 KBclaudiu.cristea
#76 typed-data-file-image-2015697-76.patch58.48 KByched
#76 interdiff.txt2.73 KByched
#68 typed-data-file-image-2015697-68.patch63.6 KBclaudiu.cristea
#59 typed-data-file-image-2015697-59.patch36.09 KBclaudiu.cristea
#56 typed-data-file-image-2015697-56.patch61.95 KByched
#56 interdiff.txt3.56 KByched
#51 typed-data-file-image-2015697-51.patch60.81 KBclaudiu.cristea
#51 interdiff.txt1.12 KBclaudiu.cristea
#48 typed-data-file-image-2015697-48.patch59.17 KBBerdir
#48 typed-data-file-image-2015697-48-interdiff.txt706 bytesBerdir
#46 interdiff.txt727 bytesclaudiu.cristea
#46 validata.png253.1 KBclaudiu.cristea
#43 typed-data-file-image-2015697-42.patch59.96 KBclaudiu.cristea
#39 typed-data-file-image-2015697-39.patch59.99 KBclaudiu.cristea
#39 interdiff.txt6.05 KBclaudiu.cristea
#37 typed-data-file-image-2015697-37.patch55.29 KBclaudiu.cristea
#37 interdiff.txt2.31 KBclaudiu.cristea
#28 typed-data-file-image-2015697-28.patch55.64 KBclaudiu.cristea
#28 interdiff.txt2.43 KBclaudiu.cristea
#22 2015697-22.patch55.44 KBclaudiu.cristea
#20 2015697-20.patch55.44 KBclaudiu.cristea
#20 interdiff.txt3.99 KBclaudiu.cristea
#19 2015697-19.patch55.37 KBclaudiu.cristea
#19 interdiff.txt6.5 KBclaudiu.cristea
#16 file-image-item-2015697-16.patch50.42 KBmsmithcti
#13 file-image-item-2015697-13.patch55.55 KBBerdir
#13 file-image-item-2015697-13-interdiff.txt1.6 KBBerdir
#12 2015697-12.patch55.34 KBclaudiu.cristea
#12 interdiff.txt7.44 KBclaudiu.cristea
#8 2015697-8.patch55.69 KBclaudiu.cristea
#8 interdiff.txt10.52 KBclaudiu.cristea
#4 2015697-4.patch51.42 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Note:
file_field_widget_uri() & file_field_widget_upload_validators() would be good candidates to move as methods on the "file field type" plugin class. They *look* like they belong to the widget classes, but in fact are more strictly about the field itself.
See #1751174-31: Convert file and image module widgets / formatters to Plugin system

swentel’s picture

Status: Postponed » Active
claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea
Status: Active » Needs work

Nobody started work on this? OK, let's grab it :)

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
51.42 KB

I moved #2015695: Convert field type to typed data plugin for image module here otherwise it's a nightmare to keep Image working while changing only Files.

To do:

This is only a starter, I expect failures.

Status: Needs review » Needs work

The last submitted patch, 2015697-4.patch, failed testing.

yched’s picture

Thanks for attacking this!
Converting image fields in the same patch definitely makes sense.

yched’s picture

Side note: the isDisplayed($item) helper method present in FileFormatterBase would be better off moved to the FileItem class.

Would make sense to do as part of this patch IMO, but this probably needs #2021817: Make widgets / formatters work on EntityNG Field value objects. to get in first.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
10.52 KB
55.69 KB

KNOWN FAILURES: All upgrade path tests are crashing. Possible explanation: https://drupal.org/node/2032369. I need some help here!

Remaining:

  • @todos from patch code: Move preparing tasks in FileWidget::massageFormValues() and validation in constrains.
claudiu.cristea’s picture

Title: Convert field type to typed data plugin for file module » Convert field type to typed data plugin for file and image modules

Fixing title.

Status: Needs review » Needs work

The last submitted patch, 2015697-8.patch, failed testing.

yched’s picture

Thanks @claudiu.cristea, will try to review soon.

Meanwhile, can you check this : #2051177: Make existing ConfigFieldItem subclasses usable by base fields, and apply it to the patch.
(i.e : use $this->getFieldDefinition()->getFieldSetting() / getFieldSettings() / getFieldLabel()
instead of $this->getInstance()->settings, $this->getInstance()->getField()->settings...)

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
7.44 KB
55.34 KB

More cleanup. I have no time to investigate the upgrade path failures but they are related to field schema. Is there a workaround?

EDIT: "I have no time" because I leave for holiday

Berdir’s picture

Yes, there is. Actually, it's IMHO not a workaround, that's exactly how it has to be and how e.g. regular table definitions are referenced too.

Status: Needs review » Needs work

The last submitted patch, file-image-item-2015697-13.patch, failed testing.

amateescu’s picture

One additional thing that we need to do in this conversion is to revert file and image classes from extending \Drupal\field\Plugin\Type\FieldType\ConfigEntityReferenceItemBase, and make them extend \Drupal\Core\Entity\Plugin\DataType\EntityReferenceItem and implement \Drupal\field\Plugin\Type\FieldType\ConfigFieldItemInterface instead.

msmithcti’s picture

I started looking at the failing tests but didn't get anywhere. Here's the reroll anyway:

Berdir’s picture

Status: Needs work » Needs review

Remember to set issues to needs review when you re-rolled.

Status: Needs review » Needs work

The last submitted patch, file-image-item-2015697-16.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
6.5 KB
55.37 KB

This should pass tests, but:

  1. I reverted usage of #2051177: Make existing ConfigFieldItem subclasses usable by base fields for ImageItem because for some reasons that breaks the default image (default value). Will research next days what's going on there.
  2. @amateescu, I didn't apply #15. I need a short discussion first to understand the case.
claudiu.cristea’s picture

Status: Needs review » Needs work

The last submitted patch, 2015697-20.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
55.44 KB

Removed trailing spaces.

interdiff.txt stays as in #20.

claudiu.cristea’s picture

Tests passed, reviews are welcomed.

claudiu.cristea’s picture

Issue tags: -Field API

#22: 2015697-22.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Field API

The last submitted patch, 2015697-22.patch, failed testing.

yched’s picture

#2060003: Wrong precedence in Field::getFieldSetting[s]() when setting appears in both field and instance got in, so we can get back to $this->getFieldSetting() in ImageItem ?

claudiu.cristea’s picture

Hold on, there's still a problem with ::getFieldSettings().

In ImageItem::settingsForm(): $this->getFieldSettings() is NOT equivalent with $this->getInstance()->getField()->getFieldSettings().

Am I missing something or this is a bug?

claudiu.cristea’s picture

Status: Needs review » Needs work
FileSize
2.43 KB
55.64 KB

Rerolled the patch and changed:

-    $settings = $this->getInstance()->getField()->settings;
+    $settings = $this->getFieldSettings();

in ImageItem.

Will fail in \Drupal\image\Tests\ImageFieldDefaultImagesTest.

claudiu.cristea’s picture

Status: Needs work » Needs review

The last submitted patch, typed-data-file-image-2015697-28.patch, failed testing.

yched’s picture

Oh crap, of course you're right.
If a field level setting & an instance level setting have the same name, the instance level takes precedence in $instance->getSettings().
And FieldItem::getFieldSetting[s]() is based on FieldItem::getFieldDefinition(), which is always an $instance in the case of a configurable field.
So there's no way to use $this->getFieldSetting[s]() in settingsForm() and get only the 'uncovered' field-level settings.

#2060003: Wrong precedence in Field::getFieldSetting[s]() when setting appears in both field and instance was right, but just moved the problem around when it comes to settingsForm() / instanceSettingsForm().
Not sure yet what's the cleanest way to solve this.

I'd really wish to be able to get rid of getInstance() when #2047229: Make use of classes for entity field and data definitions is fixed, so for now, I'd suggest something like:

// Only get field-level settings on configurable fields, since 'default_image' is present at both levels.
if ($this->getFieldDefinition() instanceof FieldInstanceInterface) {
  $settings = $this->getFieldDefinition()->getField()->getFieldSettings();
}
else {
  $settings = $this->getFieldSettings();
}
yched’s picture

Also : I still think comment #1 would be good to do as part of this patch ?

claudiu.cristea’s picture

@yched, I'll look and integrate those in the patch. In the meantime shouldn't we create a distinct issue for #31 bug? Then discuss and fix there the ::getFieldSettings() trouble. I really don't like the "if/else" approach from #31. It's a regression.

yched’s picture

Thing is, I really want to post a patch to remove the getInstance() method soon...

claudiu.cristea’s picture

An option:

  1. Take out the default settings from ::settingsForm() and ::instanceSettingsForm() and pass them to the functions as we are doing now with $has_data. Each form function would get then the default settings in the list of parameters.
  2. [Not necessary] Drop $form and $form_state from function interface.
  3. Also some default settings need some preparation (see image field 'default_image' - '#default_value' => empty($settings['default_image']) ? array() : array($settings['default_image'])). I would implement something like ::prepareFieldSettings() and ::prepareInstanceSettings() in ConfigFieldItemInterface (with implementations in FieldItemBase) so that fields can prepare values before forms will expose as default values.

Form methods would be:

  public function settingsForm(array $settings, $has_data);
  public function instanceSettingsForm(array $settings); // EDITED

or

  public function settingsForm(array $form, array &$form_state, array $settings, $has_data);
  public function instanceSettingsForm(array $form, array &$form_state, array $settings);  // EDITED

Then when calling the form methods we'll (1) get first the settings using any method, (2) pass them to preparation so that each FieldItem extension may alter them, (3) pass them to the form.

yched’s picture

I'm not in favor of #35. Passing the settings from the outside would be really weird, since they already lie in $this, and this is where the code in all the other methods fetches them.

The code proposed in #31 really is the correct approach IMO. The method needs to have special logic in case it's being used on a "configurable field definition" (FieldInstanceInterface), it's perfectly fine & valid.
It's ImageItem's decision to use settings with the same name in $field and $instance, it logically has to deal with the consequences here.

Granted, the code in #31 can be considered overly cautious since it so happens that settingsForm() will (at least currently) only ever be called for a "configurable field", and so $this->getFieldDefinition() instanceof FieldInstanceInterface will always be true. So going with just:

$settings = $this->getFieldDefinition()->getField()->getFieldSettings();

would work for now, even though it's not strictly valid interface-wise. Would just deserve a code comment - "We need the field-level 'default_image' setting, and $this->getSettings() will only provide the instance-level one, so we need to explicitly fetch the field." or something.

If in the future we started to make settings editable on "base fields" (no plans this way for now, it would require us to invent a place to store them), then ImageItem::settingsForm() would need to use the full code in #31 - which, again, is perfectly fine :-)

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.31 KB
55.29 KB

Ok, ok... although I'm still not happy with ::getFieldSettings() because this method is pretending to return correct values expected in that context. There's no indication, nowhere, that the method is returning the instance and not the field value when overlapping with the same settings name. As a conclusion I strongly believe that ::getFieldSetting() needs to be reconsidered (e.g. disallow same setting name in both filed & instance?).

claudiu.cristea’s picture

I'm looking now to #1.

claudiu.cristea’s picture

This patch, adding #1, is supposed to work but it does not. Spent few hours today but cannot realize what's going on.

The problem is when saving the node containing a file (or image) field. The node submit fails to validate.

What I discovered so far:

  1. $form_state['validate_handlers'] values are messed up. They a are losing the class name for some reason. $form_state['validate_handlers'][0] == array('', 'validate'). That first array item should be the class that owns the method "validate".
  2. If I comment out $settings = $this->getFieldSettings(); and replace $settings with a fake array in both FileItem::getUri() and ::getUploadValidators(), everything is OK.

Conclusion: Using ::getFieldSettings() (but also ::getFieldSetting()) when submitting the node form reveals a bug.

Maybe you can help with this, I didn't catch it :)

Status: Needs review » Needs work

The last submitted patch, typed-data-file-image-2015697-39.patch, failed testing.

claudiu.cristea’s picture

Now this is really weird. Locally it fails horribly as I described in #39 but I see tests are clean (with an exception -- but that seems logical). Manually tested also on simplytest.me and works. I have PHP 5.4, would this make the difference?

Just ignore all my comments from #39, I want to do some other tests.

claudiu.cristea’s picture

Status: Needs work » Needs review

Fixed tokens.

claudiu.cristea’s picture

Forgot the patch.

yched’s picture

Thanks a lot for the efforts, @claudiu.cristea, and yay for green !
Will try to review very soon.

The fails on PHP 5.4 are surprising. I don't even see where the content of #upload_validators / #upload_location would end up affecting anything in $form_state['validate_handlers']...
Not sure what our policy is regarding 5.4 only fails right now. We have no automated 5.4 testbot to prove that fixes work or avoid regressions anyway.

Well, officially this is green...

Berdir’s picture

PHP 5.4 test results of HEAD are available here: https://qa.drupal.org/pifr/test/600303

I'd very much like to avoid adding more tests, we are working on fixing the existing ones. Will try to have a look at this on 5.4/5.5 later today.

claudiu.cristea’s picture

FileSize
253.1 KB
727 bytes

Forgot the interdiff.txt, Here's for #43 against #39.

@yched, @Berdir, Not only tests are failing on my environment but the functionality itself. I cannot save a node that contains a file or image field. See this image:

validata.png

Course I checked out a fresh copy of 8.x, applied the patch and installed Drupal on a new DB. And all after restarting Apache, MySQL and even the computer. This is the same environment I used for development on several other D8 patches: MAMP running PHP 5.4.10.

I'm not dreaming, this a real bug and I recommend a test on PHP 5.4 before pushing this in.

claudiu.cristea’s picture

In https://qa.drupal.org/pifr/test/600303, under PHP 5.4.17 tab, I see the same error on Drupal\entity_reference\Tests\EntityReferenceAdminTest test.

Berdir’s picture

This fixes PHP 5.4 for me. I don't understand what exactly is happening, but assigning $this->instance seems to throw up serialization completely. Making sure it's removed before serializing is useful anyway.

claudiu.cristea’s picture

@Berdir++

Manually tested on PHP 5.4.10 and it's OK. Then ran locally all "File" and "Image" groups tests and everything is clean. I think will need a full 5.4 bot test too.

claudiu.cristea’s picture

@@ -72,4 +72,14 @@ public function getConstraints() {
+    $vars = array_keys(get_class_vars(get_class($this)));
...
+    unset($vars['instance']);

I'm afraid that $vars['instance'] doesn't exists after performing array_keys() 2 lines above. Maybe next pice of code will solve this better?

    $vars = get_class_vars(get_class($this));
    $this->instance = NULL;
    unset($vars['instance']);
    return array_keys($vars);
claudiu.cristea’s picture

Here's fix based on #50 but without $this->instance = NULL;.

yched’s picture

Thanks Berdir!

I'm puzzled by the fix though. Our tests may pass, but strictly speaking the ConfigField has now no $this->instance after being unserialized, and is technically broken. ConfigField objects need a $this->instance for its methods to work.

Still confused as to why the changes around file_field_widget_upload_validators() / file_field_widget_uri() would cause different content in stuff that's being serialized ($form / $form_state). Will try to investigate...

amateescu’s picture

Berdir’s picture

@yched: Nope, it's not broken. $this->instance is already optional by design and defaults to NULL, getInstance() loads it when required.

The reason is that those two methods call getFieldSettings(), which calls parent->getFieldDefinition(), that is just a wrapper for getInstance() and that instantiates ->instance on-demand. Once there, serialization breaks when it's not removed.

@amateescu: I'm not sure, your errors look slightly different.. but I've also seen slightly different errors here depending whether both or just a single getFieldSettings() is invoked. Let's try my interdiff there as well but I suspect it's a slightly different but similar reason.

yched’s picture

this->instance is already optional by design and defaults to NULL, getInstance() loads it when required

It's a little more complicated. getInstance() fetches $this->instance from the "official list of fields present in the configuration" *if it was not injected as part of the $definition array passed to the __construct().*

There are some cases where we need to create $item objects based on $field / $instance definition structures that differ from the "official ones from config". Field purge is one example (needs to create $item objects on "deleted" fields / instances), and the UI for "default values" will switch to that soon (needs to generate a non-required widget even if the "official" field is required - see #1988612-75: Apply formatters and widgets to rendered entity base fields, starting with node.title).
For those cases, #1969728: Implement Field API "field types" as TypedData Plugins had to implement a hackish way to inject an $instance into the $item_list / $item objects (passing the __construct() a hand-made $definition array that contains a bespoke 'instance' entry).

But I'd really want to come to a point where the FieldDefinition objects are injected into $item_list / $item objects at construct time. Having those fetch the definitions themselves is an antipattern, those should be fetched by the factory for $item objects and injected into them. We need #2047229: Make use of classes for entity field and data definitions first though, and get rid of $this->instance in favor of $this->fieldDefinition.

So I'd rather avoid removing $this->instance (future $this->fieldDefinition) at serialize time. It fixes PHP 5.4 for now, but sounds like an opportunistic workaround for a problem that lies elsewhere.

So, still pondering for now :-)

yched’s picture

Could you guys check whether by any chance the following works on 5.4 ?
(I should really set up a 5.4 env, but I need to get serious with vagrant then :-/)

Reverts ConfigFieldItem::__sleep(), and tries to deal with Field / FieldInstance serialization differently.

Berdir’s picture

Can confirm that both approaches work on 5.5 and not having either also fails with that version, so I assume the last one also works on 5.4. Nice work!

yched’s picture

Cool! It's good and bad news though, because it means Serialze interface can't be trusted if we want to support 5.3 and 5.4 - and __sleep() / __wakeup() kind of suck in comparison (see the dance this __wakeup has to do to run the __construct())

I'm also not sure what really happens here. For all I could find, 5.3 is the one where Serialize is broken (coz it relies on nested, separate serialize() calls in which objects appearing multiple times are not seen as the same object) . 5.4 is supposed to fix this. Yet 5.4 is the one that breaks here.

claudiu.cristea’s picture

Tested manually and ran "Image" & "File" group test locally on OSX, MAMP, PHP 5.4.10. Looks great, no errors.

I returned the patch from #56 with a small whitespace cleanup in \Drupal\field\Entity\Field.

claudiu.cristea’s picture

EDIT: Removed comment. Duplicate of previous.

Status: Needs review » Needs work
Issue tags: -Field API

The last submitted patch, typed-data-file-image-2015697-59.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, typed-data-file-image-2015697-59.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Field API

The last submitted patch, typed-data-file-image-2015697-59.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

Hmm. What's wrong here?

swentel’s picture

@claudiu.cristea there's a 30kb difference in the last patch, because it doesn't include the new files.

claudiu.cristea’s picture

Oh, stupid me! You're right, I left out new files :)

yched’s picture

@amateescu - just wondering: could you check whether the changes in interdiff #56 maybe also fix #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0 ?

amateescu’s picture

Couldn't apply that interdiff so I tried with the full patch from #68 and it does indeed fix that one too.

yched’s picture

@amateescu Thanks, good to know.

Then I think the serialization fixes in Field / FieldInstance would be better off in a separate issue. They still need a little more work (the array_filter() in __wakeup() is a temp hack), and the issues with Serialize in 5.3 / 5.4 deserve broader attention IMO.
The patch without them is green on 5.3, and we'd have a separate patch to fix existing issues with 5.4.
What do you guys think ?

amateescu’s picture

I would agree because the problem already exists in HEAD so it's not introduced by this patch.

Berdir’s picture

Yes, but this is going to make it much more visible. It will break editing any entity with image/file fields.

I think we either need to fix it as part of this or postpone this on the fix.

claudiu.cristea’s picture

I agree with @Berdir. Committing this patch without the serialization fix will make HEAD unusable. A lot of core contributors or testers will have to downgrade to 5.3 and I think having contributors working on 8.x with PHP 5.4 is a good thing as it has been revealed by this issue :)

So, options are: (1) committing this as it is with a followup for improving and polishing the serialization fix OR split in 2 issues (as was proposed in #71) but postpone this till serialization fix will go in core.

yched’s picture

OK, opened #2071969: Serialize interface not reliable with a slightly cleaner version of the fixes from #68.

yched’s picture

Since the serialization changes are in their own issue, here is the patch without them.

yched’s picture

I realized the serialization issues we're seeing *might* also be related to #2071997: Race condition in caching of field & instance definitions.

Hem... Could you guys check the patch over there (*without* the Field / Fieldinstance serialize() changes from #2071969: Serialize interface not reliable), see if it fixes PHP 5.4 for this patch and the entity_ref issue ? Sorry about that :-/

yched’s picture

@Berdir reported on IRC that 5.4 still breaks with #2071997: Race condition in caching of field & instance definitions, so forget #77, not related.
Patch #2071969: Serialize interface not reliable is our fix for now.

Still need to review the rest of the patch here, will try do do that soon.

yched’s picture

OK, reviewed the code. Thanks a lot for the work, @claudiu.cristea!
Many remarks, but most of them are quick fixes.

+++ b/core/modules/file/lib/Drupal/file/FileField.phpundefined
@@ -0,0 +1,72 @@
+  public function update() {
+    parent::update();
+    $this->updateFileUsage();

OK, if updateFileUsage() is more efficient dealing with all the items, then handling it at the Field class level is smarter. The lack of update() in FieldItem confused me for quite a while though. I think it would be clearer if FileItem had an empty update() method with a comment stating "File usage update is handled for all items at once in FileField::update()"

+++ b/core/modules/file/lib/Drupal/file/FileField.phpundefined
@@ -0,0 +1,72 @@
+        $target_id = $item->get('target_id')->getValue();
+        file_usage()->add(file_load($target_id), 'file', $entity->entityType(), $entity->id());

No need for $target_id & file_load(), $item->entity gives you the loaded file entity.

Although - the whole point of handling all items in the same code would be to load all the files in a multiple load instead of one by one... Would be cool in a followup, see remark below.

As a general note, the $item->target_id syntax is much handier than $item->get('target_id')->getValue(), for the exact same result :-)

+++ b/core/modules/file/lib/Drupal/file/FileField.phpundefined
@@ -0,0 +1,72 @@
+      $target_id = $item->get('target_id')->getValue();
+      $current_fids[] = $target_id;

$current_fids[] = $item->target_id

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.phpundefined
@@ -0,0 +1,362 @@
+ *   module = "file",

Not needed anymore (same for ImageItem)

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.phpundefined
@@ -0,0 +1,362 @@
+      '#element_validate' => array(array($this, 'validateDirectory')),

Yay for the code of validateDirectory() moving in the class, but we should use static methods for FAPI #callbacks whenever possible.
'#validate' => array($this, 'validateDirectory')); adds unnecessary headaches when the $form gets serialized.
validateDirectory()
validateExtensions()
validateMaxFilesize()
should be static, and added to the form with '#validate' => array(get_class($this), 'validateDirectory'));

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.phpundefined
@@ -0,0 +1,362 @@
+      '#default_value' => isset($settings['description_field']) ? $settings['description_field'] : '',

I know it's present in HEAD, but the isset() is not needed, '#default_value' => $settings['description_field'] should just work

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.phpundefined
@@ -0,0 +1,362 @@
+    file_usage()->add(file_load($target_id), 'file', $entity->entityType(), $entity->id());

If those insert() / delete() ... methods stay here (item per item), then $this->entity rather than file_load().

But it seems those would deserve multi-item / multi-load treatment in FielField too ?
Doing those multi-load optimizations would be ok in a followup, since HEAD currently does separate file_load() calls. Opened #2073033: Optimize file usage updates in file/image fields for that. Would be really cool if someone could tackle it once this is in :-)

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.phpundefined
@@ -0,0 +1,362 @@
+   * Render API callback: Validates the file destination field.

Copy/paste from HEAD, but rather "Form API callback" ?
(same for the other helper)

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.phpundefined
@@ -0,0 +1,362 @@
+   * This function is assigned as an #element_validate callback in
+   * finstanceSettingsForm().

typo finstanceSettingsForm() :-)

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.phpundefined
@@ -0,0 +1,362 @@
+  public function getUri($data = array()) {

Actually, getUploadLocation() would be more accurate, what we return here is not a URI for a specific file item.

+++ b/core/modules/image/lib/Drupal/image/Plugin/field/field_type/ImageItem.phpundefined
@@ -0,0 +1,327 @@
+          'description' => 'The ID of the target entity.',

FileItem::schema() says "The ID of the file entity", so I guess we should do the same here ?

+++ b/core/modules/image/lib/Drupal/image/Plugin/field/field_type/ImageItem.phpundefined
@@ -0,0 +1,327 @@
+    $this->definition['settings']['target_type'] = 'image';

Should be 'file', not 'image' (there is no 'image' entity type) (the current code in HEAD has 'file')

+++ b/core/modules/image/lib/Drupal/image/Plugin/field/field_type/ImageItem.phpundefined
@@ -0,0 +1,327 @@
+    $settings = $this->getFieldDefinition()->getField()->getFieldSettings();

So yeah :-). As I wrote in #36 above, this would really need a comment IMO.

+++ b/core/modules/image/lib/Drupal/image/Plugin/field/field_type/ImageItem.phpundefined
@@ -0,0 +1,327 @@
+    // When the user sets the scheme on the UI, even for the first time, it's
+    // updating a field because fields are created on the "Manage fields"
+    // page.
+    $element['default_image'] = array(

This comment looks out of place / stale now. I think we can remove it (it would also apply to FileItem::settingsForm(), and it's absent over there).
(+ 80 chars wrapping is off, last word could fit in the previous line)

+++ b/core/modules/image/lib/Drupal/image/Plugin/field/field_type/ImageItem.phpundefined
@@ -0,0 +1,327 @@
+      '#element_validate' => array(array($this, 'validateResolution')),

Same as above, static method please.

+++ b/core/modules/image/lib/Drupal/image/Plugin/field/field_type/ImageItem.phpundefined
@@ -0,0 +1,327 @@
+    $entity = $this->getRoot();

Not used in the code ?

+++ b/core/modules/image/lib/Drupal/image/Plugin/field/field_type/ImageItem.phpundefined
@@ -0,0 +1,327 @@
+      $image = \Drupal::service('image.factory')->get(file_load($target_id)->getFileUri());

$this->entity rather than file_load() (& the $target_id var can go away)

But would benefit from a multi-load treatment at the Field level too...

yched’s picture

Also, opened another issue for additional cleanup in file.field.inc / image.field.inc : #2072995: Move FAPI callbacks for file/image widgets in classes

yched’s picture

+++ b/core/modules/file/lib/Drupal/file/FileField.phpundefined
@@ -0,0 +1,72 @@
+namespace Drupal\file;

I'd vote to put the Field classes in the same folders than thier FieldItem classes, rather than lost at the toplevel of the lib folder.

Berdir’s picture

Berdir’s picture

Status: Needs review » Needs work

Great review, also wondering if there's something we can simplify now that the entity_reference type has been committed?

claudiu.cristea’s picture

Status: Needs work » Postponed

@yched, @Berdir,

I will fix the patch based on #79 and #81 and tackle also #2073033: Optimize file usage updates in file/image fields and #2072995: Move FAPI callbacks for file/image widgets in classes. But this one will have to be postponed until #2071969: Serialize interface not reliable.

Not only because I'm using PHP 5.4 (and I don't want to downgrade as long as http://php.net/archive/2013.php#id2013-07-11-1) but pushing this in (without #2071969: Serialize interface not reliable) will make HEAD unusable for all contributors using PHP >=5.4. And, as I said before, we need them contributing on >=5.4 platform as long as we have 5.4 test coverage only for committed code.

Postponing till #2071969: Serialize interface not reliable.

claudiu.cristea’s picture

@yched, and many thanks for the review. Great, indeed!

yched’s picture

Awesome, thanks @claudiu.cristea :)
And yes, it was agreed that #2071969: Serialize interface not reliable needs to get in first. I'll bump over there.

claudiu.cristea’s picture

Issue tags: +PHP 5.4
yched’s picture

Note: #2056405: Let field types provide their support for "default values" and associated input UI added FileField and ImageField classes already, so the patch should just add methods in there rather than create new files now.
(those classes should still be moved along next to the new location of the FileItem / ImageItem classes, though)

Berdir’s picture

Status: Postponed » Needs work

And this can be unpostponed, yay :)

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -PHP 5.4
FileSize
15.91 KB
59.99 KB

@yched, @Berdir,

Commenting mostly on #79, #81 and #83.

@yched

+++ b/core/modules/file/lib/Drupal/file/FileField.phpundefined
@@ -0,0 +1,72 @@
+  public function update() {
+    parent::update();
+    $this->updateFileUsage();

[...] The lack of update() in FieldItem confused me for quite a while though. I think it would be clearer if FileItem had an empty update() method with a comment stating "File usage update is handled for all items at once in FileField::update()"

I agree with you but that is only from the reviewer perspective. When you reviewed the conversion, you didn't find any correspondence of hook_field_update() in the place where you expected to be :) I don't think that it's a real problem. Also there's a followup to move everything there: #2073033: Optimize file usage updates in file/image fields.

@yched

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.phpundefined
@@ -0,0 +1,362 @@
+      '#default_value' => isset($settings['description_field']) ? $settings['description_field'] : '',

I know it's present in HEAD, but the isset() is not needed, '#default_value' => $settings['description_field'] should just work

It should but it doesn't. It fails with Undefined index: description_field but only in image tests. This happens (I think) because in ImageItem::instanceSettingsForm() we call the parent (FileItem) instance settings form but settings for images contain no 'description_field':

class ImageItem extends FileItem {
  ...
  public function instanceSettingsForm(array $form, array &$form_state) {
    // Get base form from FileItem::instanceSettingsForm().
    $element = parent::instanceSettingsForm($form, $form_state);

    ...

I know, I know... it's very ugly to workaround in a parent class for a child implementation class. Or just drop $element = parent::instanceSettingsForm($form, $form_state) and duplicate needed form elements?

@Berdir
[...] also wondering if there's something we can simplify now that the entity_reference type has been committed?

Can you review the possibility of such improvement of the patch?

yched’s picture

The lack of update() in FieldItem - I agree with you but that is only from the reviewer perspective

Not really IMO, simply looking at the FileItem class and seeing it handles insert() / delete() and not update() is confusing, this is not about the old code. But agreed, #2073033: Optimize file usage updates in file/image fields will reshuffle that anyway.

- isset($settings['description_field'])
True, my bad. This is another example of #2050113: PHP notice on multiple items image field. Never mind then.

- ImageItem::instanceSettingsForm() / parent::instanceSettingsForm()
Doesn't really bother me - that's what the current code does, just in a non-OO way.

Beware of #88 - core now has existing FileField and ImageField classes.
Your patch creates new ones but doesn't delete the old ones. It also needs to make sure that the code in the current classes is ported over to the new ones.

Berdir’s picture

Status: Needs review » Needs work
FileSize
21.96 KB

So, that's what we have now, with this patch:
reference_fields.png

(taxonomy term reference is not yet converted, but does use the same base class)

One thing that could help avoid some weird issues might be instead of image extending file, to have a common base class for them. Just an idea, not sure if that makes sense.

claudiu.cristea’s picture

@yched, forgot about #88. This mean I have to extend FileField from LegacyConfigField instead of how it was defined in my patch (from ConfigField)?

yched’s picture

@claudiu.cristea: no, disconnecting LegacyConfigField / LegacyConfigFieldItem is the exact purpose of this patch ;-)

What this means is that the patch should not create a brand new FileField.php class, but:
- move the existing file/lib/Drupal/file/Type/FileField.php (was added recently in core) to file/lib/Drupal/file/Plugin/field/field_type/FileField.php
- make it extend ConfigField instead of LegacyConfigField
- add your new update() / updateFileUsage() methods there

And similar for ImageField.php.
Your patch chose to directly use the FileField class for image fields so far, but since core now has an ImageField.php, let's use it.
Needs to be moved to image/lib/Drupal/image/Plugin/field/field_type/ImageField.php, and extend FileField instead of LegacyConfigField.
(also means ImageItem's 'list_class' annotation should be updated to ImageField)

Berdir’s picture

Why do we need ImageField and FileField, given that thy both have the same, empty implementation?

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
61.64 KB
1.89 KB

@yched, I don't see any reason why we need a class that does nothing. IMO we need to drop ImageField and simply use FileField as 'list_class'.

claudiu.cristea’s picture

Oh! Cross posting, so I got a wrong patch name :)

yched’s picture

My reasoning is that ImageField provides an implementation of defaultValueForm() that's empty, for reasons of its own, not related to FileField. It so happens that FileField, for reasons of its own, also provides an implementation of defaultValueForm() that's empty, but that's a coincidence.
No biggie, I can live with either :-)

yched’s picture

Also, I find it vaguely unsettling that a field type directly uses the field class of another field type - but maybe it's just me, again, no biggie.

claudiu.cristea’s picture

Also, I find it vaguely unsettling that a field type directly uses the field class of another field type - but maybe it's just me, again, no biggie.

This was something that drawn my attention too. But images are special cases of files, ImageItem is extending FileItem. Annotation fields are not inheritable. If it would have been, then it would make perfectly sense :)

claudiu.cristea’s picture

As it turned green, this needs more reviews or just RTBC.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php
    @@ -0,0 +1,75 @@
    +  protected function updateFileUsage() {
    

    Missing documentation.

  2. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php
    @@ -0,0 +1,75 @@
    +    // Get the field id.
    +    $field_id = $this->getInstance()->getField()->id();
    

    @yched: Does that other issue only remove the method from the public interface, or do we need to change this?

  3. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php
    @@ -0,0 +1,75 @@
    +    $original = $entity->original->getBCEntity();
    

    Let's rewrite this to not use the BC decorator, this was only necessary because not all types where NG yet.

    Remove that call, and use $original->getTranslation($langcode)->$field_*name* (the field id is now "node.field_image" or something like that. needs to change to getFieldName(). Actually, that means the code above could use getFieldDefinition()->getFieldName() I think.

  4. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php
    @@ -0,0 +1,75 @@
    +    $langcode = $original->language()->id;
    

    I think this could be wrong when an entity has multiple translations/languages. Not exactly sure how it would be correctly, especially with the sync stuff. But still, I'd expect this to be per language? (getTranslationLanguages()). @plach, @yched?

  5. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.php
    @@ -0,0 +1,361 @@
    +    $this->definition['settings']['target_type'] = 'file';
    +    // Definitions vary by entity type and bundle, so key them accordingly.
    +    $key = $this->definition['settings']['target_type'] . ':';
    +    $key .= isset($this->definition['settings']['target_bundle']) ? $this->definition['settings']['target_bundle'] : '';
    

    This was already like this before, but this code is kinda bogus, file/image references don't have target bundles, they're always about file. So this cache thing shouldn't need to be by $key.

  6. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.php
    @@ -0,0 +1,361 @@
    +  public function insert() {
    

    I agree with @yched that it's weird to have insert() and delete() implementations here and update() somewhere else. +1 on adding a comment. Fine with that being inside insert.

  7. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.php
    @@ -0,0 +1,361 @@
    +    $target_id = $this->get('target_id')->getValue();
    ...
    +    $target_id = $this->get('target_id')->getValue();
    

    use $this->target_id. That avoids the creation of a primitive typed data object just to get the value again. I'd like to have a method instead of only __get() but we currently don't and I don't know how to name it :)

  8. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.php
    @@ -0,0 +1,361 @@
    +    // Add a new usage for this new uploaded file.
    +    file_usage()->add(file_load($target_id), 'file', $entity->entityType(), $entity->id());
    

    Same here, not sure how this works with entity translations.

  9. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.php
    @@ -0,0 +1,361 @@
    +    $target_id = $this->get('target_id')->getValue();
    +    return empty($target_id);
    

    empty($this->target_id) should work as well here.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
7.7 KB
60.8 KB

Thank you, @Berdir

#102.6: I added some @todos because, anyway, we will move that functionally to FileField in #2073033: Optimize file usage updates in file/image fields.

yched’s picture

Thanks folks, I think we're almost :) there !

  1. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php
    @@ -0,0 +1,72 @@
    +    if (!empty($entity->original->$field_name)) {
    +      foreach ($entity->original->$field_name as $item) {
    

    the !empty() check shouldn't be needed I think.

    Then there's a bigger issue, as @berdir pointed in #102.4: the previous code in file_field_update() received a $langcode, and compared with the values in $entity->original *in this langcode*. Now, FileField::update() receives no $langcode anymore, and it's not too clear which language current patch uses to check the field values in $entity->original.

    The issue is that right now Field objects don't know their langcode. #2078625: Field/FieldItem value objects should carry their language should address that, but meanwhile, I'd suggest we do here as we're doing for #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property, that is: temporarily use the $langcode of the entity, and add a @todo on #2078625: Field/FieldItem value objects should carry their language.
    // @todo We need the langcode of the current values. For now, assume it is
    // the langcode of the parent entity, and revisit after
    // https://drupal.org/node/2078625.
    $langcode = $entity->language()->id;
    foreach ($entity->original->getTranslation($langcode)->$field_name as $item) {...}

    Re @Berdir #102.8: I don't think language considerations are an issue for the other file_usage() calls. We're incrementing or decrementing usage counters, so language doesn't matter - as long as each language adds its own +1/-1, which is the case.

  2. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileItem.php
    @@ -0,0 +1,357 @@
    +  public function getPropertyDefinitions() {
    +    $this->definition['settings']['target_type'] = 'file';
    

    Now that I think of it, this looks really weird...
    This is currently present in HEAD, so not a blocker here - opened #2079517: Weird assignment of definition['settings']['target_type'] in FieldItem::getPropertyDefinitions()

  3. +++ b/core/modules/image/lib/Drupal/image/Plugin/field/field_type/ImageItem.php
    @@ -0,0 +1,322 @@
    +    $width = $this->get('width')->getValue();
    +    $height = $this->get('height')->getValue();
    

    I'd advocate we move all of those from ->get($property)->getValue() to $this->$property.
    Didn't scan the while patch to find if there were others left aside from those two.

yched’s picture

re: my own #104, first item:

#2078625: Field/FieldItem value objects should carry their language got in, so this code:

FileField.php:
+    if (!empty($entity->original->$field_name)) {
+      foreach ($entity->original->$field_name as $item) {
+        $original_fids[] = $item->target_id;
+        if (isset($item->target_id) && !in_array($item->target_id, $fids)) {
+          // Decrement the file usage count by 1.
+          file_usage()->delete($item->entity, 'file', $entity->entityType(), $entity->id());
+        }
+      }
+    }

should now be:

   foreach ($entity->original->getTranslation($this->getLangcode())->$field_name as $item) {
     $original_fids[] = $item->target_id;
     // ...
   }
yched’s picture

Status: Needs review » Needs work

(@Berdir: are you still eligible to RTBC this patch ? If so, I can reroll with the changes from #104 / #105, since @claudiu.cristea doesn't seem to be around right now...)

Berdir’s picture

Hm, I fixed the upgrade path all the way back in #13 and the __sleep() stuff that I worked on already got in, so I guess so... if you can confirm that the upgrade patches changes are good, which I think are in sync with what we've done elsewhere :)

yched’s picture

Status: Needs work » Needs review
FileSize
2.41 KB
58.98 KB

OK, updated patch for #104/#105 then.

And yes, I vet the upgrade path changes :-)

Status: Needs review » Needs work

The last submitted patch, typed-data-file-image-2015697-108.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
925 bytes
58.98 KB

Sigh...

Status: Needs review » Needs work

The last submitted patch, typed-data-file-image-2015697-110.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
924 bytes
58.98 KB

Friday night - or "How to make a fool of yourself in two patches."

plach’s picture

Priority: Normal » Critical

Marking critical as this is blocking #1983554: Remove BC-mode from EntityNG .

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php
@@ -51,13 +51,12 @@ protected function updateFileUsage() {
+      if (isset($item->target_id) && !in_array($item->target_id, $fids)) {

Nitpick: The isset() is not necessary, as target_id is always there.

Feel free to provide a re-roll for this but I think this is ready to go. This got multiple reviews by @yched and me, we have some follow-ups and it's blocking the BC decorator removal (or we'd have to re-do the changes in here there and unnecessarily conflict with this).

yched’s picture

Indeed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, typed-data-file-image-2015697-115.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
59 KB

Nope, there are actually cases where $item->target_id can be present and not stand for a real entity.
This looks like an occurrence of the weird / very annoying "empty field but sill with an initialized item at offset 0" behavior I've been seeing from time to time :-/

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in.

claudiu.cristea’s picture

Oh, it seems I missed the final battle of this war -- I was out last days :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looked through this and couldn't find anything to complain about. It's nice that image field finally inherits from file field, that's been wanted for years!

Committed/pushed to 8.x.

smiletrl’s picture

Status: Fixed » Needs review
FileSize
808 bytes

Let's clean it a bit. getInstance() is removed in favour of getFieldDefinition(). Luckily, getInstance() is not actually used in this case, but it would be good that it's removed, perhaps?

smiletrl’s picture

Status: Needs review » Closed (fixed)
yched’s picture

claudiu.cristea’s picture

Assigned: claudiu.cristea » Unassigned