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.
Testbot playground for #1969728: Implement Field API "field types" as TypedData Plugins
Comment | File | Size | Author |
---|---|---|---|
#200 | d8_field_types.patch.interdiff.txt | 862 bytes | fago |
#200 | d8_field_types.patch | 314.09 KB | fago |
#198 | d8_field_types.patch.interdiff.txt | 17.09 KB | fago |
#198 | d8_field_types.patch | 313.25 KB | fago |
#195 | d8_field_types.patch.interdiff.txt | 15.49 KB | fago |
Comments
Comment #1
yched CreditAttribution: yched commentedLet's see where this brings us.
(interdiff is with @swentel's #1969728-28: Implement Field API "field types" as TypedData Plugins)
Comment #3
yched CreditAttribution: yched commentedSome more fixes.
Comment #5
yched CreditAttribution: yched commentedGoing after the FormTest fails tomorrow. Looks like the underlying code accounts for a fair part of the fails.
Comment #6
swentel CreditAttribution: swentel commentedI've started some debugging on this and adding the test field on a node type works fine, when using the test_entity, submission fails, so it has *something* todo with entity types I guess, but rather lost at the moment.
Just enable the field_test module to test locally - to get the field on the test entity, I just copy pasted the functions and definitions from FormTest in a function in Drupal to run once.
Comment #7
yched CreditAttribution: yched commentedShould fix at least a couple of form-related fails.
Bottomline is: the TypedDataManager::getPropertyInstance() method used by WidgetBase only works for NG entity types.
Added a different version for non-NG entity types.
(+ commented out FieldAttachOtherTest::testFieldAttachValidate(), that tests behavior from field_default_validate(), that is not ported yet)
Comment #9
yched CreditAttribution: yched commentedShould fix fails in taxo/TermTest - EntityNG is *so* complicated :-/
+ commented out more tests on stuff that's not ported yet (validation, translation...)
Comment #11
yched CreditAttribution: yched commentedSo basically, the field type hooks / methods were not called on BC entities so far.
This should fix a bunch of fails on tests using non-NG entities - well, unless it breaks havoc :-)
Comment #12
yched CreditAttribution: yched commentedWhile we're at it, let's try this.
Comment #13
yched CreditAttribution: yched commentedNote: LinkItemTest fails will need a discussion with EntityNG folks in Portland, that's kind of intrinsically broken right now.
Comment #15
yched CreditAttribution: yched commentedSimple reroll for now.
Comment #16
yched CreditAttribution: yched commentedComment #18
yched CreditAttribution: yched commentedLet's see if this is better.
Comment #20
yched CreditAttribution: yched commentedCan't work. Rather, this.
Comment #21
yched CreditAttribution: yched commentedShould fix a bunch of notices.
Comment #23
yched CreditAttribution: yched commentedShould fix most upgrade fails. I hate you, update_module_enable().
(Basically, including hackish fixes from #1941000: update_module_enable() does not update ModuleHandler::$moduleList)
Comment #25
yched CreditAttribution: yched commentedChecking something.
Comment #27
yched CreditAttribution: yched commentedSpent hours on figuring out why EntityReferenceItemTest was just stopping right in the middle of entity_create(), with no error message or something.
...broken "use" statements at the top of ConfigurableEntityReferenceItem. Gee.
Comment #29
yched CreditAttribution: yched commented#27: field-types-as-datatypes-testbot-1992138-27.patch queued for re-testing.
Comment #31
yched CreditAttribution: yched commentedAttempt at cleaning up some hacks.
Comment #33
yched CreditAttribution: yched commentedMore fixes
Comment #35
yched CreditAttribution: yched commentedAfter discussing with Berdir, better fix for the FileFieldRevisionTest fails - see #1893820-20: Manage entity field definitions in the entity manager for more details.
With a bit of luck, might even fix other fails ?
Comment #36
yched CreditAttribution: yched commentedMore fixes
Comment #38
yched CreditAttribution: yched commentedChecking the reroll.
Comment #40
yched CreditAttribution: yched commentedPrevious reroll skipped some commenting out of validation-related tests.
+ new patch fixes Link tests, by moving serialization / unserialization to field_sql_storage, as agreed with @fago in Portland.
Not much time to push this further this week. Could use some help with the remaining fails:
- EntityReferenceAutoCreateTest: probably needs someone familiar with entity_ref autocreate widget
- EntityTranslationSyncImageTest: not sure how to deal with that, FieldTranslationSynchronizer::synchronizeItems() is pretty tied to receiving field values as an array keyed by language, probably needs some refactoring :-/
Comment #42
yched CreditAttribution: yched commentedReroll.
Comment #43
amateescu CreditAttribution: amateescu commentedI investigated the EntityReferenceAutoCreateTest failure, there's no problem with the ER autocomplete widget, this test is actually catching a bug: the $items argument received by a hook_field_presave() implementation is always an empty array! Also, it's not specific to ER's hook implementation, the same thing happens in taxonomy_field_presave() for example.
I tried to debug the root cause, got to Drupal\Core\Entity\Field\Type\Field::getValue() and then got lost in this typed data hell..
Comment #44
BerdirThis passes that test but haven't checked other things yet and it will not work with non-NG entities yet but that's the direction where auto-create should go I think.
Instead of messing around with special target_id's, and the special label property, this simply creates the new entity already in the widget and then just saves it in presave. terms are already basically doing the same.
Comment #46
amateescu CreditAttribution: amateescu commentedYep, that will fix the test, but the problem with empty $items in hook_field_presave() still remains..
Comment #47
BerdirThey were empty because is_empty() filtered them out, they shouldn't be empty anymore, otherwise ->save() would be called and the test wouldn't pass?
Comment #48
yched CreditAttribution: yched commentedYay, thanks a ton Berdir !
This is roughly similar with what the autocomplete widget for taxo terms does, so consistency ++
(except the taxo widget uses WidgetInterface::massageFormValues(), which would be a better fit for the job - not changing this here, opened #2007422: entity_reference autocomplete widget should use massageFormValues() rather than #element_validate)
Refactored a bit (see attached interdiff, relative to the changes in #44)
- Determine the uid upstream, and pass it to the createNewEntity() method
- move to \Drupal::entityManager()
- Couldn't prevent myself from changing a string var from $entity to $input in AutocompleteTagsWidget::elementValidate()
and pushed to the sandbox.
Comment #49
yched CreditAttribution: yched commentedComment #50
amateescu CreditAttribution: amateescu commentedI discussed this change a bit with Berdir today. Yves, would you mind if we move the entire interdiff from #48 to a new issue? It'd be nice to get it in earlier and it's not really the scope of this patch to refactor ER's auto_create handling :)
In any case, me-- for not being helpful at all..
Comment #51
yched CreditAttribution: yched commented@amateescu : sure, no worries - then maybe tackle #2007422: entity_reference autocomplete widget should use massageFormValues() rather than #element_validate at the same time ?
I'll keep the fix in the patch meanwhile, so that we stay close to green :-)
[edit: actually, that would be the cumulative interdiff from #44 and #48]
Comment #52
yched CreditAttribution: yched commentedAbout EntityTranslationSyncImageTest fails and FieldTranslationSynchronizer:
The first problem is that synchronizeFields() previously ran within hook_field_attach_presave() (which swapped the $entity with a BC entity), and now runs within hook_entity_presave(). But the helper synchronizeItems() method fatals out when provided an NG Field object.
So the first fix is to add back that BC logic within synchronizeFields(), which already has some. Updated patch does that.
Fixing this lets the synchronizeFields() method run and the $entity-save() can go on, but causes more issues down the road during update() on the image Field.
No time to investigate further. Might be that synchronizeFields() now doesn't die but doesn't do its job properly either, or something else...
[edit: mh, tesbot seems down, sweet]
Comment #53
yched CreditAttribution: yched commentedSo, it seems synchronizeFields() does its job correctly on the BC entity: $entity->field_test_et_ui, as an array of values keyed by language, seems to have the correct values.
But when the entity then reaches invokeHook('update'),
- $entity->values->field_test_et_ui is what you would expect (2 arrays of file items in two languages)
- But the corresponding (file) Field object only has $this->list = array(0 => array('entity' => NULL));
That's all I can say for now and til next weekend.
No clue why it works in HEAD and not here, the only difference I see is:
- in HEAD, synchronizeFields() is called from hook_field_attach_presave(), that passes a BC entity
- with patch synchronizeFields() is called from hook_entity_presave(), and takes care of switching to a BC entity itself internally.
Any hints welcome :-)
Comment #54
plachI'll try to have a look to the failures during the weekend.
Comment #56
yched CreditAttribution: yched commentedPosting the work on the new shape of:
- hook_field_validate() (moves to FieldItem::getConstrainst() and Symfony validation)
- hook_field_load() (moves to a non-multiple FieldItem::prepareCache() method),
as discussed with @fago and@effulgentsia in Portland
Not fully done yet, and tests will most definitely fail, but checking where we stand.
On my local setup, it seems to fix the EntityTranslationSyncImageTest fails discussed above - that's totally uncalled for, and I still have no clue why, but I'll happily accept the gift :-)
Comment #58
yched CreditAttribution: yched commented#56: field-types-as-datatypes-testbot-1992138-56.patch queued for re-testing.
Comment #59
yched CreditAttribution: yched commented- remove a couple debug validation constraints
- add back missing constraint for text summary length
- separate EntityStorageConstroller::invokeFieldMethod() from invokeHook()
- Doh, fix stupid logic mistake, FieldItem methods (preSave(), update()...) were not called anymore.
(with this fix, EntityTranslationSyncImageTest does fail again, scratch #56 :-/)
As if it wasn't painful enough to get the tesbot to spit out results, it would probably help if didn't upload blatantly broken patches...
Comment #61
yched CreditAttribution: yched commentedYet another stupid mistake.
Comment #62
yched CreditAttribution: yched commentedGetting there.
Comment #64
yched CreditAttribution: yched commentedFYI - @amateescu, @Berdir: I just bumped #2007422: entity_reference autocomplete widget should use massageFormValues() rather than #element_validate to 'major'. The current flow of the "auto create" feature on the autocomplete widget is inconsistent with the definition of the 'target_id' property as a positive int, and breaks when we start to actually validate() stuff.
Comment #65
yched CreditAttribution: yched commented- Fixed fatals with entity_reference (missing prepareCache() method from the interface)
- Workaround for the "entity reference widget auto create" fails mentioned in #64
- Move tests about "nested entity forms" out of the way or now. Not sure if/how this can be supported now.
Comment #67
yched CreditAttribution: yched commented- Get rid of one hardcoded call to field_info_instances() in DatabaseStorageController
- TypedDataTest needs the field.info service to be available
- ConfigurableEntityReferenceItem::isEmpty() cannot rely on the legacy bridge to hook_field_is_empty(), this loads the referenced entity, and causes a nasty infinite loop in EntityResolverTest.
- The workaround for ConfigurableEntityReferenceItem validation fails (see #2007422: entity_reference autocomplete widget should use massageFormValues() rather than #element_validate) causes other fails in other places. Make it more specific.
Comment #68
yched CreditAttribution: yched commentedLittle summary on the remaining fails:
- BulkDeleteTest fails just count the # of times hook_field_load() is called, and the hook is deperecated now. Not worried about those, will be easy to adapt (possibly even ditch...)
- OptionsDynamicValuesValidationTest,
- TermFieldTest,
- TextFieldTest,
- FieldAttachOtherTest
Those fail because they test field_attach_validate() directly, and the function doesn't exist anymore :-). This needs to move to $item->validate(), but doing so is convoluted while those tests use BC entities.
The first three classes above should be easy to fix after #2004224: Convert tests using TestEntity to EntityTest (except Field API tests) gets in - @Berdir++.
Less sure how I'm going to address FieldAttachOtherTest, moving this one to an NG entity is going to be more involved.
--> Still remain:
- our old friend EntityTranslationSyncImageTest :-)
- Drupal\field\Tests\FormTest::testNestedFieldForm() (commented out for now)
I have a train journey tonight, will try to look further into those.
Comment #70
BerdirMost of the field attach and other field API tests are converted to NG as well in #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest, will require a major re-roll after #2004224: Convert tests using TestEntity to EntityTest (except Field API tests) is in, though. Will do that once that the other issue is in. Feel free to take over anything you'll need from there then. I think the validate stuff was working quite nicely with NG.
Comment #71
plachSorry for being vacant. I will be able to review the field sync code tomorrow.
Comment #72
yched CreditAttribution: yched commented@plach: thanks !
From what I could tell with the latest patch, looking at this snippet in EntityTranslationSyncImageTest:
$assert is false. What is true is : $item['alt'] == $source_item['alt'] && $item['title'] == $source_item['title']
In other words, the 'alt' and 'title' in $default_langcode have been synchronized with those from $langcode, while they would apparently have been expected to stay unchanged.
Comment #73
yched CreditAttribution: yched commentedUpdated patch:
- @fago: One problem is that the validation errors raised by the explicit or implicit constraints found in FieldItem::getPropertyDefinitions() (e.g 'type' => 'uri' in LinkItem) do not point to any specific delta (the propertyPath is just the name of the property, e.g. 'url', not '0[url]').
So the code in WidgetBase that assigns errors back to a specific form element triggered notices.
This will need to be fixed in the validation process (there is a "@todo: Figure out how to handle nested constraint structures as collections." in TypedDataManager::getConstraints() that seems to be exactly that ?), but this is above my head right now.
For now, added a workaround in WidgetBase, those errors are assigned to delta 0. That's incorrect, but good enough to trick existing tests.
- @Berdir: For now, took care of the validation tests in FieldAttachOtherTest by moving them to a separate test class, that uses the NG test_entity :-)
Comment #75
yched CreditAttribution: yched commentedFinally found the cause of the errors on the autocomplete widgets, opened #2012662: Constraints on 'target_id' / 'tid' properties break autocomplete if applied for this.
Updated patch contains the fix from over there.
Comment #77
plachAfter a looong debugging session I was able to track down the failure: the reason is that presave image field hook is called on empty values under certain conditions. This causes a mismatch between item hashes that causes the synchronization to fail, because
width
andheight
columns are not populated.The reason is that in
DatabaseStorageController
, line 724 (patch applied), we have the correct item values in$entity->values
but not in$entity->fields
, which is empty, hence the presave hook is not executed.Not sure how to fix this.
Comment #78
yched CreditAttribution: yched commented@plach: thanks a lot ! Processing this.
Meanwhile, additional fixes for #2012662: Constraints on 'target_id' / 'tid' properties break autocomplete if applied - the formatters need to be adjusted to the new convention for "no id yet, entity being auto-created".
Comment #80
yched CreditAttribution: yched commentedReroll.
Comment #81
yched CreditAttribution: yched commentedAdapt validation flow after #2002152: Implement entity validation based on symfony validator.
Comment #83
yched CreditAttribution: yched commentedOur legacy discovery cannot rely on HookDiscovery anymore, since this now fires Exceptions whithin upgrades.
Comment #84
yched CreditAttribution: yched commentedDoh, wrong git command.
Comment #86
yched CreditAttribution: yched commentedAs discussed with @fago and @effulgentsia:
add the PrepareCacheInterface, and only call FieldItem::prepareCache() when needed.
Comment #88
yched CreditAttribution: yched commentedFixes the "nested entity form" test (was commented out so far).
- This steals a part of #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest, to make sure testNestedFieldForm() runs on the NG entity_test rather than test_entity.
- This preserves field_attach_form_validate() only to support this explicit use case until #1728816: Ensure multiple entity forms can be embedded into one form gets figured out.
Couldn't reproduce the SimpleTestTest fails reported in the previous run, assuming bot hiccup ?
Comment #90
yched CreditAttribution: yched commentedForgot to remove the old testNestedFieldForm().
Interdiff with #86.
Comment #92
yched CreditAttribution: yched commentedCode simplification.
Comment #94
yched CreditAttribution: yched commentedFixed field purge, by making the $instance injectable at needed places in the callstack
Let's see if this breaks other stuff.
Comment #96
yched CreditAttribution: yched commentedShould fix the fails introduced above.
Comment #98
yched CreditAttribution: yched commented#96: field-types-as-datatypes-testbot-1992138-96.patch queued for re-testing.
Comment #100
yched CreditAttribution: yched commentedDrupal\views\Tests\Plugin\PagerTest
Drupal\views_ui\Tests\DefaultViewsTest
Those very much look like random fails
#96: field-types-as-datatypes-testbot-1992138-96.patch queued for re-testing.
Comment #102
yched CreditAttribution: yched commentedReroll
Comment #104
amateescu CreditAttribution: amateescu commentedOpened #2016177: Regression: Refactor 'autocreate entity' handling from entity_reference with the combined interdiffs from #44 and #48. Will rtbc when the bot comes back as it's not my code there :)
Comment #105
yched CreditAttribution: yched commentedReroll after #2004224: Convert tests using TestEntity to EntityTest (except Field API tests) + fixes for the last validation-related tests (OptionsDynamicValuesValidationTest, TermFieldTest, TextFieldTest)
(+ pushed to the sandbox)
Comment #107
yched CreditAttribution: yched commentedFix issues with field purge (manipulating values of fields that do not officially exist)
Comment #109
yched CreditAttribution: yched commentedReroll, + remove some test code that was not supposed to stay there.
Comment #110
yched CreditAttribution: yched commentedI can haz tests ?
Comment #112
plachI know I know, I am working on the field sync stuff tonight :)
Comment #113
yched CreditAttribution: yched commentedLOL
Comment #114
yched CreditAttribution: yched commentedReroll, + cleanup after #1986802: rename PluginInspectionInterface::getDefinition() to getPluginDefinition()
Comment #116
yched CreditAttribution: yched commentedDamn - need to run, will look into this later tonight
Comment #117
yched CreditAttribution: yched commentedPartially reverts #114.
TypedDataInterface cannot extend PluginInspectionInterface, because Entity implements TypedDataInterface but cannot provide the PluginInspectionInterface methods.
Comment #119
yched CreditAttribution: yched commentedRename a bunch of classes, using "C" as an abbrev. will never fly.
Comment #121
yched CreditAttribution: yched commented#119: field-types-as-datatypes-testbot-1992138-119.patch queued for re-testing.
Comment #122
yched CreditAttribution: yched commentedChecking where we stand with prepareView()
(expect lots of fails)
Comment #124
yched CreditAttribution: yched commentedChecking the reroll of the "no-prepareView()" patch after #2013837: Rewrite field sync on top of NG entities / #2016177: Regression: Refactor 'autocreate entity' handling from entity_reference
30k less :-)
Comment #125
yched CreditAttribution: yched commentedMore hairpulling issues with the 'entity' property of taxo / entity_ref fields :-(
Checking something.
Comment #127
yched CreditAttribution: yched commentedback to checking prepareView() - field_view_field() ported.
(interdiff with #122)
Comment #129
yched CreditAttribution: yched commentedFound a couple fixes.
Comment #131
yched CreditAttribution: yched commentedAdded missing BC support for invokeFieldMethodMultiple()
Comment #133
yched CreditAttribution: yched commentedStupid mistake.
Comment #134
fagosome validation stuff.. -> testing
Comment #135
fagoyched: Could you grant me access to the field API sandbox? So I could push my changes to a field-types-as-datatypes-1969728-fago branch there?
Comment #136
yched CreditAttribution: yched commented@fago: you have commit access already, it seems ?
Comment #137
fagooh - indeed, thanks.
Here is an interdiff/patch for the available translation issue.
Pushed those two patches into
field-types-as-datatypes-1969728-fago (first patch)
field-types-as-datatypes-1969728-translation (this patch)
Comment #138
yched CreditAttribution: yched commentedprepareView() / invokeFieldMethodMultiple: odd behavior in $entity->getTranslation()->get().
Silly workaround for now, let's see where this gets us.
Comment #140
yched CreditAttribution: yched commented@fago: merged field-types-as-datatypes-1969728-fago / #134 in the main branch. Awesome !
(I kept the debug constraints code in text::getConstraints() for now though)
Comment #141
yched CreditAttribution: yched commentedChecking the reroll after #1893772: Move entity-type specific storage logic into entity classes and #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets.
Comment #143
fagook, here is my complete interdiff (3commits).
Comment #144
fagoHere is a variant containing only the first two commits (to see if new fails got introduced, where.)
Comment #145
fagoAnd lastly, here is just
Use getInstance() method to lazy-load and initialize field instances.
Comment #147
fago#144: d8_field_types.patch queued for re-testing.
Comment #148
fagook, so this one changes #144 to keep catching exceptions.
Note, I've pushed this commits to field-types-as-datatypes-1969728-fago2
Comment #149
yched CreditAttribution: yched commentedThanks a lot @fago, and triple yay at the 'entity' fixes !!
[edited my post out: I finally realized that you kept ConfigField __construct() where the $instance can still be injected - awesome :-D
I merged your changes in the main branch]
Comment #150
fagore-rolled patch and fixed some merge conflicts.
Comment #152
fagocontact conversion has shown that we broke e-mail field to work without e-mail module, attached interdiff fixes that issue. Let's whether that's all.
Comment #153
fagoand this converts configurable field type manager to entity field type manager.
Comment #155
fago#153: d8_field_types.patch queued for re-testing.
Comment #157
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll for #1965208: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem, plus as part of that, changed TaxonomyTermReferenceItem to extend ConfigurableEntityReferenceItem (not sure if that's correct or not).
Comment #159
amateescu CreditAttribution: amateescu commentedNot at all, we have #1847596: Remove Taxonomy term reference field in favor of Entity reference for that.
Comment #160
effulgentsia CreditAttribution: effulgentsia commentedAlright. I'm just digging myself into a deeper and deeper hole here. Here's some terrible changes that manage to get a Standard profile install to fail later. But it still fails. Maybe fago can help get us out of this hole.
Note: there's two holes here. What to do with TaxonomyTermReferenceItem. I guess it needs to copy the implementation of ConfigFieldItemInterface from ConfigurableEntityReferenceItem rather than extending it, though that's an unfortunate code duplication. The other hole is the need to clearCachedDefinitions() for image fields. That just points to there being a bug somewhere else in this patch related to the cache not being cleared elsewhere.
Comment #162
fagooh, I think that's correct. I don't see how it would work else. #1847596: Remove Taxonomy term reference field in favor of Entity reference is something different as it tries to do away with the term-reference field.
ok, so I re-rolled the patch as well.
(@effulgentsia: sry, that was quicker for me then converting your patch to Git. Could you push further changes also into the field API sandbox, e.g. using a branch field-types-as-datatypes-1969728-eff, so we can easily merge back and forth.)
Problem with just extending is that taxonomy module does not depend on ER module yet. As we should not solve #1847596: Remove Taxonomy term reference field in favor of Entity reference here, so I just extracted the ER class into a common ConfigEntityReferenceItemBase provided by field module for now. That seems to do the job, as term reference field tests are green for me.
@install-fail: Strange, the standard installation profile test works for me when invoked with the run-test.sh script.
Comment #163
fagoComment #165
amateescu CreditAttribution: amateescu commentedI'm sorry guys but I have to strongly disagree here. TaxonomyTermReferenceItem does not have *anything* to do with ConfigurableEntityReferenceItem. The "duplicate" code exists in HEAD right now, and that's something that #1847596: Remove Taxonomy term reference field in favor of Entity reference is trying to fix.
There are some talks about doing it with derivatives (in #1847590: Fix UX of entity reference field type selection) but it's really not decided yet so, please, let's not scope-creep that discussion in this patch!
The intention with #1965208: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem was only to have a commont target_id column/definition on *Reference fields.
I would honestly prefer to roll it back than to see this mess..
Comment #166
fagohm, I don't really get where's the problem with that? Did you look at the interdiff I posted and ConfigEntityReferenceItemBase? It's not like it's extending the whole ER-code, this still lives in callbacks - it's just extending the logic to invoke the callbacks. Consequently, I do not think this changes the way ER and taxonomy-reference interact compared to HEAD.
@drupal-install-failed: Weird, as said standard-install tests work for me. I don't have time to debug this more right now, so help would be welcome!
Comment #167
amateescu CreditAttribution: amateescu commentedSorry, I was pretty swamped this morning so I only skimmed through the last two comments. The interdiff in #162 looks good indeed, not what I expected. My apologies.. :)
Comment #168
fagoNo worries :)
Comment #169
effulgentsia CreditAttribution: effulgentsia commentedI don't know if this is the correct place to clear the cache, but this makes Standard profile install for me via the browser. Sorry, just attaching an interdiff for now. I'll set up a local git clone if/when I do more serious work on this issue.
Comment #170
fagoAwesome - thanks!
As this is entity related I think it should be part of entity_info_cache_clear(), so moved it over there.
Comment #172
fagodamn - looks like the ordering is important here.
Comment #174
fagook, reverted my changes. So this is the Git branch + effulgentsia's interdiff.
Comment #175
fagoComment #177
fago#174: d8_field_types.patch queued for re-testing.
Comment #178
andypostNot sure that fieldability should affect translation. This was removed for menu_link entity that none-fieldable by default but needs translation
Comment #179
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll.
Comment #181
effulgentsia CreditAttribution: effulgentsia commentedFixed for #1893820: Manage entity field definitions in the entity manager.
Comment #183
effulgentsia CreditAttribution: effulgentsia commentedOh, the woes of copy/pasting.
Comment #184
effulgentsia CreditAttribution: effulgentsia commentedAnd even worse, rolling a patch incorrectly. Interdiff in #183 is correct, but here's the actual patch.
Comment #186
effulgentsia CreditAttribution: effulgentsia commentedMissed one.
Comment #187
msonnabaum CreditAttribution: msonnabaum commentedThis looks like a bit of a performance regression :(
On a node page with 14 comments:
The majority of it is in field_attach_prepare_view:
Comment #188
effulgentsia CreditAttribution: effulgentsia commented@andypost: can you copy your feedback from #178 to #1969728: Implement Field API "field types" as TypedData Plugins? I considered doing it myself, but figured it would be good for the comment in the main issue to list you as the author of it.
Comment #189
effulgentsia CreditAttribution: effulgentsia commented#186: d8_field_types-186.patch queued for re-testing.
Comment #191
effulgentsia CreditAttribution: effulgentsia commentedI'm confused why #1969728-172: Implement Field API "field types" as TypedData Plugins is failing so much. Trying the same patch out here.
Comment #192
effulgentsia CreditAttribution: effulgentsia commentedComment #193
fagoI had a look at the performance problem and identified two issues:
- EntityNG::language() is called a lot and re-created the language object each time. Added a static cache for that. (9000x times on my 300comments test page).
- invokeFieldMethodMultiple() seems to be heavy as it runs through all fields on all entities and used quite some methods per combination - so I've optimized that a bit.
- invokeFieldMethodMultiple() still is heavy as it leads to objects being created for all fields and field items that have not been used previously. As long as view() uses them alter on, that's fine - but I suppose we'll have some not-used fields as well (comment->hostname, or homepage if it's not anonymous, etc.), so I guess it would make sense to add the same interface-check approach to prepareView() as we have for prepareCache() (not done yet)
Updated patch, let's see what bot says.
Comment #194
fagochecked performance of #193 with xdebug disabled on a node page rendering 300comments: While #191 was on that page a regression of 38%, #191 is "only" a regression of 14,3%. So it's already a considerable improvement.
Comment #195
fagoUpdated patch addressing reviews of #167-#169 except for re-naming classes (follow-up) and improving text-field item's code separation.
Comment #197
fago#195: d8_field_types.patch queued for re-testing.
Comment #198
fagook, so I took a stab on separate prepareView() into a separate interface + calling it only conditionally also. However, when doing so I noticed that the two only implementations in core are for image + file fields - all others are using formatters. Image+file fields use it for pre-loading the referenced entities, while ER does exactly the same it does it in formatter prepareView in a base class. So looking at that, I think it makes a lot of sense to do it the ER-field way when common prepareView logic is needed: Provide a formatter base class for it.
Thus, instead of adding the interface to conditionally have prepareView logic in the field class - attached patch moves that into according formatter-base classes, what makes a lot of sense to me.
Given that change, I ran my performance test again and regression is down to -4,96% - i.e. it's actually a performance improvement of ca. 5%.
Comment #200
fagomissed the picture test, with this one it's hopefully green again.
Comment #201
effulgentsia CreditAttribution: effulgentsia commentedFrom yched (whose internet access currently prevents him from posting to this directly):
Hook_field_prepare_view() was introduced in D7 to have a place run common logic for values of a that we didn't want to trust each formatter (including 3rd party or custom formatters) to get right all by themselves.
This included loading file/images, or, more importantly, checking node acces for node_reference (not sure how ER handles this currently).
By having a field-type-specific, formatter-agnostic step that runs before we let formatters go wild, we were sure that no poorly coded formatter could mess sensitive things up.
I'm a bit suprised that just removing the invokeFieldMethodMultiple('prepareView') call goes from -15% to +5% perf impact, somthing looks weird here.
However, the fact that formatters are plugins/classes in D8 (as compared to the D7 callback-based api) means it's much easier to provide base functionnality in a base class, as fago did in the last patch, and the reasoning above applies much less. AAMOF, it's probably conceptually much cleaner to keep all display-related logic in the formatter land whith inheritance and base classes.
In short: +1, awesome !
Comment #202
effulgentsia CreditAttribution: effulgentsia commentedWere those numbers generated with the 'features.comment_user_picture' theme setting on or off? I wonder how much of an impact template_preprocess_comment()'s
$variables['user_picture'] = user_view($account, 'compact');
is having, since that's running once per comment, rather than as a multiple.Comment #203
fagoGreat to hear you like it.
With defaults, i.e. on. Yeah, I guess the performance plus comes from saving time in not having to prepare values for prepare-view-field here and for comments.
I got this when having it disabled and the patch applied:
Page execution time was 4414.29 ms. Memory used at: devel_boot()=7.3 MB, devel_shutdown()=49.22 MB, PHP peak=56.5 MB.
The difference stems from the objects we avoid to instantiate for *each* comment being rendered, i.e. non-configurable fields (comment hostname field + item) that are not rendered/used but got instantiated with the previous patch due to prepare view. Applying that for each comment, it sums up.
Comment #204
fago