Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Depends on #1969728: Implement Field API "field types" as TypedData Plugins
Instructions are on #2014671: [META] Convert all field types to plugins
Comment | File | Size | Author |
---|---|---|---|
#121 | field_type_clean-2015697-121.patch | 808 bytes | smiletrl |
#117 | typed-data-file-image-2015697-117.patch | 59 KB | yched |
#117 | interdiff.txt | 1.21 KB | yched |
#115 | typed-data-file-image-2015697-115.patch | 58.96 KB | yched |
#115 | interdiff.txt | 856 bytes | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedNote:
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
Comment #2
swentel CreditAttribution: swentel commentedComment #3
claudiu.cristeaNobody started work on this? OK, let's grab it :)
Comment #4
claudiu.cristeaI 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.
Comment #6
yched CreditAttribution: yched commentedThanks for attacking this!
Converting image fields in the same patch definitely makes sense.
Comment #7
yched CreditAttribution: yched commentedSide 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.
Comment #8
claudiu.cristeaKNOWN FAILURES: All upgrade path tests are crashing. Possible explanation: https://drupal.org/node/2032369. I need some help here!
Remaining:
@todo
s from patch code: Move preparing tasks inFileWidget::massageFormValues()
and validation in constrains.Comment #9
claudiu.cristeaFixing title.
Comment #11
yched CreditAttribution: yched commentedThanks @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...)
Comment #12
claudiu.cristeaMore 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
Comment #13
BerdirYes, 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.
Comment #15
amateescu CreditAttribution: amateescu commentedOne 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.Comment #16
msmithcti CreditAttribution: msmithcti commentedI started looking at the failing tests but didn't get anywhere. Here's the reroll anyway:
Comment #17
BerdirRemember to set issues to needs review when you re-rolled.
Comment #19
claudiu.cristeaThis should pass tests, but:
ImageItem
because for some reasons that breaks the default image (default value). Will research next days what's going on there.Comment #20
claudiu.cristeaFixed #15 and opened #2060003: Wrong precedence in Field::getFieldSetting[s]() when setting appears in both field and instance for field settings default image.
Comment #22
claudiu.cristeaRemoved trailing spaces.
interdiff.txt stays as in #20.
Comment #23
claudiu.cristeaTests passed, reviews are welcomed.
Comment #24
claudiu.cristea#22: 2015697-22.patch queued for re-testing.
Comment #26
yched CreditAttribution: yched commented#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 ?
Comment #27
claudiu.cristeaHold 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?
Comment #28
claudiu.cristeaRerolled the patch and changed:
in
ImageItem
.Will fail in
\Drupal\image\Tests\ImageFieldDefaultImagesTest
.Comment #29
claudiu.cristeaComment #31
yched CreditAttribution: yched commentedOh 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:
Comment #32
yched CreditAttribution: yched commentedAlso : I still think comment #1 would be good to do as part of this patch ?
Comment #33
claudiu.cristea@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.Comment #34
yched CreditAttribution: yched commentedThing is, I really want to post a patch to remove the getInstance() method soon...
Comment #35
claudiu.cristeaAn option:
::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.$form
and$form_state
from function interface.'#default_value' => empty($settings['default_image']) ? array() : array($settings['default_image'])
). I would implement something like::prepareFieldSettings()
and::prepareInstanceSettings()
inConfigFieldItemInterface
(with implementations inFieldItemBase
) so that fields can prepare values before forms will expose as default values.Form methods would be:
or
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.
Comment #36
yched CreditAttribution: yched commentedI'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: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 :-)
Comment #37
claudiu.cristeaOk, 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?).Comment #38
claudiu.cristeaI'm looking now to #1.
Comment #39
claudiu.cristeaThis 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:
$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".$settings = $this->getFieldSettings();
and replace $settings with a fake array in bothFileItem::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 :)
Comment #41
claudiu.cristeaNow 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.
Comment #42
claudiu.cristeaFixed tokens.
Comment #43
claudiu.cristeaForgot the patch.
Comment #44
yched CreditAttribution: yched commentedThanks 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...
Comment #45
BerdirPHP 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.
Comment #46
claudiu.cristeaForgot 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:
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.
Comment #47
claudiu.cristeaIn 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.Comment #48
BerdirThis 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.
Comment #49
claudiu.cristea@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.
Comment #50
claudiu.cristeaI'm afraid that
$vars['instance']
doesn't exists after performingarray_keys()
2 lines above. Maybe next pice of code will solve this better?Comment #51
claudiu.cristeaHere's fix based on #50 but without
$this->instance = NULL;
.Comment #52
yched CreditAttribution: yched commentedThanks 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...
Comment #53
amateescu CreditAttribution: amateescu commentedThis is probably very similar to #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0.
Comment #54
Berdir@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.
Comment #55
yched CreditAttribution: yched commentedIt'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 :-)
Comment #56
yched CreditAttribution: yched commentedCould 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.
Comment #57
BerdirCan 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!
Comment #58
yched CreditAttribution: yched commentedCool! 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.
Comment #59
claudiu.cristeaTested 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.
Comment #60
claudiu.cristeaEDIT: Removed comment. Duplicate of previous.
Comment #62
claudiu.cristea#59: typed-data-file-image-2015697-59.patch queued for re-testing.
Comment #64
claudiu.cristea#59: typed-data-file-image-2015697-59.patch queued for re-testing.
Comment #66
claudiu.cristeaHmm. What's wrong here?
Comment #67
swentel CreditAttribution: swentel commented@claudiu.cristea there's a 30kb difference in the last patch, because it doesn't include the new files.
Comment #68
claudiu.cristeaOh, stupid me! You're right, I left out new files :)
Comment #69
yched CreditAttribution: yched commented@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 ?
Comment #70
amateescu CreditAttribution: amateescu commentedCouldn't apply that interdiff so I tried with the full patch from #68 and it does indeed fix that one too.
Comment #71
yched CreditAttribution: yched commented@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 ?
Comment #72
amateescu CreditAttribution: amateescu commentedI would agree because the problem already exists in HEAD so it's not introduced by this patch.
Comment #73
BerdirYes, 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.
Comment #74
claudiu.cristeaI 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.
Comment #75
yched CreditAttribution: yched commentedOK, opened #2071969: Serialize interface not reliable with a slightly cleaner version of the fixes from #68.
Comment #76
yched CreditAttribution: yched commentedSince the serialization changes are in their own issue, here is the patch without them.
Comment #77
yched CreditAttribution: yched commentedI 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 :-/
Comment #78
yched CreditAttribution: yched commented@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.
Comment #79
yched CreditAttribution: yched commentedOK, reviewed the code. Thanks a lot for the work, @claudiu.cristea!
Many remarks, but most of them are quick fixes.
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()"
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 :-)
$current_fids[] = $item->target_id
Not needed anymore (same for ImageItem)
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'));
I know it's present in HEAD, but the isset() is not needed, '#default_value' => $settings['description_field'] should just work
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 :-)
Copy/paste from HEAD, but rather "Form API callback" ?
(same for the other helper)
typo finstanceSettingsForm() :-)
Actually, getUploadLocation() would be more accurate, what we return here is not a URI for a specific file item.
FileItem::schema() says "The ID of the file entity", so I guess we should do the same here ?
Should be 'file', not 'image' (there is no 'image' entity type) (the current code in HEAD has 'file')
So yeah :-). As I wrote in #36 above, this would really need a comment IMO.
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)
Same as above, static method please.
Not used in the code ?
$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...
Comment #80
yched CreditAttribution: yched commentedAlso, opened another issue for additional cleanup in file.field.inc / image.field.inc : #2072995: Move FAPI callbacks for file/image widgets in classes
Comment #81
yched CreditAttribution: yched commentedI'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.
Comment #82
Berdir#76: typed-data-file-image-2015697-76.patch queued for re-testing.
Comment #83
BerdirGreat review, also wondering if there's something we can simplify now that the entity_reference type has been committed?
Comment #84
claudiu.cristea@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.
Comment #85
claudiu.cristea@yched, and many thanks for the review. Great, indeed!
Comment #86
yched CreditAttribution: yched commentedAwesome, thanks @claudiu.cristea :)
And yes, it was agreed that #2071969: Serialize interface not reliable needs to get in first. I'll bump over there.
Comment #87
claudiu.cristeaComment #88
yched CreditAttribution: yched commentedNote: #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)
Comment #89
BerdirAnd this can be unpostponed, yay :)
Comment #90
claudiu.cristea@yched, @Berdir,
Commenting mostly on #79, #81 and #83.
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.It should but it doesn't. It fails with
Undefined index: description_field
but only in image tests. This happens (I think) because inImageItem::instanceSettingsForm()
we call the parent (FileItem) instance settings form but settings for images contain no 'description_field':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?Can you review the possibility of such improvement of the patch?
Comment #91
yched CreditAttribution: yched commentedNot 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.
Comment #92
BerdirSo, that's what we have now, with this patch:
(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.
Comment #93
claudiu.cristea@yched, forgot about #88. This mean I have to extend FileField from
LegacyConfigField
instead of how it was defined in my patch (fromConfigField
)?Comment #94
yched CreditAttribution: yched commented@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)
Comment #95
BerdirWhy do we need ImageField and FileField, given that thy both have the same, empty implementation?
Comment #96
claudiu.cristea@yched, I don't see any reason why we need a class that does nothing. IMO we need to drop
ImageField
and simply useFileField
as'list_class'
.Comment #97
claudiu.cristeaOh! Cross posting, so I got a wrong patch name :)
Comment #98
yched CreditAttribution: yched commentedMy 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 :-)
Comment #99
yched CreditAttribution: yched commentedAlso, 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.
Comment #100
claudiu.cristeaThis was something that drawn my attention too. But images are special cases of files,
ImageItem
is extendingFileItem
. Annotation fields are not inheritable. If it would have been, then it would make perfectly sense :)Comment #101
claudiu.cristeaAs it turned green, this needs more reviews or just RTBC.
Comment #102
BerdirMissing documentation.
@yched: Does that other issue only remove the method from the public interface, or do we need to change this?
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.
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?
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.
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.
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 :)
Same here, not sure how this works with entity translations.
empty($this->target_id) should work as well here.
Comment #103
claudiu.cristeaThank you, @Berdir
#102.6: I added some
@todo
s because, anyway, we will move that functionally to FileField in #2073033: Optimize file usage updates in file/image fields.Comment #104
yched CreditAttribution: yched commentedThanks folks, I think we're almost :) there !
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.
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()
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.
Comment #105
yched CreditAttribution: yched commentedre: my own #104, first item:
#2078625: Field/FieldItem value objects should carry their language got in, so this code:
should now be:
Comment #106
yched CreditAttribution: yched commented(@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...)
Comment #107
BerdirHm, 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 :)
Comment #108
yched CreditAttribution: yched commentedOK, updated patch for #104/#105 then.
And yes, I vet the upgrade path changes :-)
Comment #110
yched CreditAttribution: yched commentedSigh...
Comment #112
yched CreditAttribution: yched commentedFriday night - or "How to make a fool of yourself in two patches."
Comment #113
plachMarking critical as this is blocking #1983554: Remove BC-mode from EntityNG .
Comment #114
BerdirNitpick: 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).
Comment #115
yched CreditAttribution: yched commentedIndeed.
Comment #117
yched CreditAttribution: yched commentedNope, 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 :-/
Comment #118
swentel CreditAttribution: swentel commentedLet's get this in.
Comment #119
claudiu.cristeaOh, it seems I missed the final battle of this war -- I was out last days :)
Comment #120
catchLooked 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.
Comment #121
smiletrl CreditAttribution: smiletrl commentedLet'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?
Comment #122
smiletrl CreditAttribution: smiletrl commentedsee #2089485: [follow-up]: Clean up for file field type..
Comment #123
yched CreditAttribution: yched commentedFYI : #2090619: Move file visibility check to the FileItem class
Comment #124
claudiu.cristea