Files: 
CommentFileSizeAuthor
#121 field_type_clean-2015697-121.patch808 bytessmiletrl
FAILED: [[SimpleTest]]: [MySQL] 58,821 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
#117 typed-data-file-image-2015697-117.patch59 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,453 pass(es).
[ View ]
#117 interdiff.txt1.21 KByched
#115 typed-data-file-image-2015697-115.patch58.96 KByched
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#115 interdiff.txt856 bytesyched
#112 typed-data-file-image-2015697-112.patch58.98 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,885 pass(es).
[ View ]
#112 interdiff.txt924 bytesyched
#110 typed-data-file-image-2015697-110.patch58.98 KByched
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#110 interdiff.txt925 bytesyched
#108 typed-data-file-image-2015697-108.patch58.98 KByched
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php.
[ View ]
#108 interdiff.txt2.41 KByched
#103 typed-data-file-image-2015697-103.patch60.8 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,415 pass(es).
[ View ]
#103 interdiff.txt7.7 KBclaudiu.cristea
#96 interdiff.txt1.89 KBclaudiu.cristea
#96 typed-data-file-image-2015697-95.patch61.64 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,095 pass(es).
[ View ]
#92 reference_fields.png21.96 KBBerdir
#90 typed-data-file-image-2015697-90.patch59.99 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,372 pass(es).
[ View ]
#90 interdiff.txt15.91 KBclaudiu.cristea
#76 typed-data-file-image-2015697-76.patch58.48 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,463 pass(es).
[ View ]
#76 interdiff.txt2.73 KByched
#68 typed-data-file-image-2015697-68.patch63.6 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,266 pass(es).
[ View ]
#59 typed-data-file-image-2015697-59.patch36.09 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#56 typed-data-file-image-2015697-56.patch61.95 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,261 pass(es).
[ View ]
#56 interdiff.txt3.56 KByched
#51 typed-data-file-image-2015697-51.patch60.81 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,221 pass(es).
[ View ]
#51 interdiff.txt1.12 KBclaudiu.cristea
#48 typed-data-file-image-2015697-48.patch59.17 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,937 pass(es).
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 57,898 pass(es).
[ View ]
#39 typed-data-file-image-2015697-39.patch59.99 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 58,165 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#39 interdiff.txt6.05 KBclaudiu.cristea
#37 typed-data-file-image-2015697-37.patch55.29 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,866 pass(es).
[ View ]
#37 interdiff.txt2.31 KBclaudiu.cristea
#28 typed-data-file-image-2015697-28.patch55.64 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 58,162 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#28 interdiff.txt2.43 KBclaudiu.cristea
#22 2015697-22.patch55.44 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2015697-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 2015697-20.patch55.44 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#20 interdiff.txt3.99 KBclaudiu.cristea
#19 2015697-19.patch55.37 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,010 pass(es).
[ View ]
#19 interdiff.txt6.5 KBclaudiu.cristea
#16 file-image-item-2015697-16.patch50.42 KBsplatio
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#13 file-image-item-2015697-13.patch55.55 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,184 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#13 file-image-item-2015697-13-interdiff.txt1.6 KBBerdir
#12 2015697-12.patch55.34 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#12 interdiff.txt7.44 KBclaudiu.cristea
#8 2015697-8.patch55.69 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 57,199 pass(es), 9 fail(s), and 16 exception(s).
[ View ]
#8 interdiff.txt10.52 KBclaudiu.cristea
#4 2015697-4.patch51.42 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 56,725 pass(es), 22 fail(s), and 23 exception(s).
[ View ]

Comments

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

Status:Postponed» Active

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

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

Status:Needs work» Needs review
StatusFileSize
new51.42 KB
FAILED: [[SimpleTest]]: [MySQL] 56,725 pass(es), 22 fail(s), and 23 exception(s).
[ View ]

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new10.52 KB
new55.69 KB
FAILED: [[SimpleTest]]: [MySQL] 57,199 pass(es), 9 fail(s), and 16 exception(s).
[ View ]

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.

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.

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...)

Status:Needs work» Needs review
StatusFileSize
new7.44 KB
new55.34 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new1.6 KB
new55.55 KB
FAILED: [[SimpleTest]]: [MySQL] 57,184 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

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.

StatusFileSize
new50.42 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new6.5 KB
new55.37 KB
PASSED: [[SimpleTest]]: [MySQL] 58,010 pass(es).
[ View ]

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.

StatusFileSize
new3.99 KB
new55.44 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new55.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2015697-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Removed trailing spaces.

interdiff.txt stays as in #20.

Tests passed, reviews are welcomed.

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.

#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 ?

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?

Status:Needs review» Needs work
StatusFileSize
new2.43 KB
new55.64 KB
FAILED: [[SimpleTest]]: [MySQL] 58,162 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Rerolled the patch and changed:

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

in ImageItem.

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

Status:Needs work» Needs review

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

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();
}

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

@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.

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

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:

<?php
 
public function settingsForm(array $settings, $has_data);
  public function
instanceSettingsForm(array $settings); // EDITED
?>

or

<?php
 
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.

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 :-)

Status:Needs work» Needs review
StatusFileSize
new2.31 KB
new55.29 KB
PASSED: [[SimpleTest]]: [MySQL] 57,866 pass(es).
[ View ]

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?).

I'm looking now to #1.

StatusFileSize
new6.05 KB
new59.99 KB
FAILED: [[SimpleTest]]: [MySQL] 58,165 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review

Fixed tokens.

StatusFileSize
new59.96 KB
PASSED: [[SimpleTest]]: [MySQL] 57,898 pass(es).
[ View ]

Forgot the patch.

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...

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.

StatusFileSize
new253.1 KB
new727 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.

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.

StatusFileSize
new706 bytes
new59.17 KB
PASSED: [[SimpleTest]]: [MySQL] 57,937 pass(es).
[ View ]

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.

@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.

@@ -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?

<?php
    $vars
= get_class_vars(get_class($this));
   
$this->instance = NULL;
    unset(
$vars['instance']);
    return
array_keys($vars);
?>

StatusFileSize
new1.12 KB
new60.81 KB
PASSED: [[SimpleTest]]: [MySQL] 58,221 pass(es).
[ View ]

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

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...

@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.

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 :-)

StatusFileSize
new3.56 KB
new61.95 KB
PASSED: [[SimpleTest]]: [MySQL] 58,261 pass(es).
[ View ]

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.

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!

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.

StatusFileSize
new36.09 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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.

Status:Needs work» Needs review

Hmm. What's wrong here?

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

StatusFileSize
new63.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,266 pass(es).
[ View ]

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

@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 ?

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

@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 ?

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

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.

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.

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

StatusFileSize
new2.73 KB
new58.48 KB
PASSED: [[SimpleTest]]: [MySQL] 58,463 pass(es).
[ View ]

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

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 :-/

@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.

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...

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

+++ 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.

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?

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: Change notice: 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.

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

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.

Issue tags:+PHP 5.4

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)

Status:Postponed» Needs work

And this can be unpostponed, yay :)

Status:Needs work» Needs review
Issue tags:-PHP 5.4
StatusFileSize
new15.91 KB
new59.99 KB
PASSED: [[SimpleTest]]: [MySQL] 58,372 pass(es).
[ View ]

@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':

<?php
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?

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.

Status:Needs review» Needs work
StatusFileSize
new21.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.

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

@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)

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

Status:Needs work» Needs review
StatusFileSize
new61.64 KB
PASSED: [[SimpleTest]]: [MySQL] 58,095 pass(es).
[ View ]
new1.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'.

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

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 :-)

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.

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 :)

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

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.

Status:Needs work» Needs review
StatusFileSize
new7.7 KB
new60.8 KB
PASSED: [[SimpleTest]]: [MySQL] 58,415 pass(es).
[ View ]

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.

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.

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;
     // ...
   }

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...)

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 :)

Status:Needs work» Needs review
StatusFileSize
new2.41 KB
new58.98 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new925 bytes
new58.98 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Sigh...

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new924 bytes
new58.98 KB
PASSED: [[SimpleTest]]: [MySQL] 58,885 pass(es).
[ View ]

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

Priority:Normal» Critical

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

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).

StatusFileSize
new856 bytes
new58.96 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Indeed.

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
new59 KB
PASSED: [[SimpleTest]]: [MySQL] 58,453 pass(es).
[ View ]

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 :-/

Status:Needs review» Reviewed & tested by the community

Let's get this in.

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

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.

Status:Fixed» Needs review
StatusFileSize
new808 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,821 pass(es), 6 fail(s), and 2 exception(s).
[ View ]

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?

Status:Needs review» Closed (fixed)

Assigned:claudiu.cristea» Unassigned