CommentFileSizeAuthor
#200 d8_field_types.patch.interdiff.txt862 bytesfago
#200 d8_field_types.patch314.09 KBfago
#198 d8_field_types.patch.interdiff.txt17.09 KBfago
#198 d8_field_types.patch313.25 KBfago
#195 d8_field_types.patch.interdiff.txt15.49 KBfago
#195 d8_field_types.patch305.85 KBfago
#193 d8_field_types.patch306.13 KBfago
#193 d8_field_types.patch.interdiff.txt4.25 KBfago
#191 d8_field_types-1969728-172.patch304.74 KBeffulgentsia
#187 1992138-summary.png44.33 KBmsonnabaum
#187 1992138-field_attach_prepare_view-summary.png74.89 KBmsonnabaum
#187 1992138-field_attach_prepare_view-calls.png112.15 KBmsonnabaum
#186 d8_field_types-186.patch304.66 KBeffulgentsia
#186 interdiff.txt1.04 KBeffulgentsia
#184 d8_field_types-184.patch304.77 KBeffulgentsia
#183 d8_field_types-183.patch304.79 KBeffulgentsia
#183 interdiff.txt1.58 KBeffulgentsia
#181 d8_field_types-181.patch304.79 KBeffulgentsia
#181 interdiff.txt6.66 KBeffulgentsia
#179 d8_field_types-179.patch305.47 KBeffulgentsia
#174 d8_field_types.patch305.71 KBfago
#172 d8_field_types.interdiff.txt624 bytesfago
#172 d8_field_types.patch305.51 KBfago
#170 d8_field_types.patch305.51 KBfago
#170 d8_field_types.interdiff.txt1.25 KBfago
#169 d8_field_types-169.patch305.99 KBeffulgentsia
#169 interdiff.txt426 byteseffulgentsia
#162 d8_field_types.interdiff.txt14.71 KBfago
#162 d8_field_types.patch305.62 KBfago
#160 d8_field_types-160.patch303.48 KBeffulgentsia
#160 interdiff.txt1006 byteseffulgentsia
#157 d8_field_types-157.patch302.68 KBeffulgentsia
#153 d8_field_types.patch302.4 KBfago
#153 d8_field_types.interdiff.txt48.78 KBfago
#152 d8_field_types.interdiff.txt1.21 KBfago
#152 d8_field_types.patch302.07 KBfago
#150 d8_field_types.patch301.47 KBfago
#148 d8_field_types.patch284.57 KBfago
#148 d8_field_types.patch.interdiff.txt1.11 KBfago
#145 d8_field_types.patch.interdiff.txt17.33 KBfago
#145 d8_field_types.patch282.68 KBfago
#144 d8_field_types.patch.interdiff.txt18.43 KBfago
#144 d8_field_types.patch282.41 KBfago
#143 d8_field_types.patch284.29 KBfago
#143 d8_field_types.patch.interdiff.txt20.31 KBfago
#141 field-types-as-datatypes-testbot-1992138-141.patch282.98 KByched
#138 field-types-as-datatypes-testbot-1992138-138.patch266.45 KByched
#138 interdiff.txt1.39 KByched
#137 d8_field_types.patch.interdiff.txt1.89 KBfago
#137 d8_field_types.patch258.66 KBfago
#134 d8_field_types.patch.interdiff.txt8.58 KBfago
#134 d8_field_types.patch258.43 KBfago
#133 field-types-as-datatypes-testbot-1992138-133.patch266.22 KByched
#133 interdiff.txt1.9 KByched
#131 field-types-as-datatypes-testbot-1992138-131.patch266.67 KByched
#131 interdiff.txt2.04 KByched
#129 field-types-as-datatypes-testbot-1992138-128.patch265.68 KByched
#129 interdiff.txt2.78 KByched
#127 field-types-as-datatypes-testbot-1992138-127.patch263.45 KByched
#127 interdiff.txt3.55 KByched
#125 field-types-as-datatypes-testbot-1992138-125.patch257.35 KByched
#124 field-types-as-datatypes-testbot-1992138-124.patch256.85 KByched
#122 field-types-as-datatypes-testbot-1992138-122.patch283.64 KByched
#119 field-types-as-datatypes-testbot-1992138-119.patch280.31 KByched
#119 interdiff.txt37.69 KByched
#117 field-types-as-datatypes-testbot-1992138-117.patch267.42 KByched
#117 interdiff.txt1.34 KByched
#114 field-types-as-datatypes-testbot-1992138-114.patch267.94 KByched
#114 interdiff.txt1.79 KByched
#110 field-types-as-datatypes-testbot-1992138-109.patch265.62 KByched
#109 field-types-as-datatypes-testbot-1992138-109.patch265.62 KByched
#109 interdiff.txt641 bytesyched
#107 field-types-as-datatypes-testbot-1992138-106.patch268.69 KByched
#107 interdiff.txt12.28 KByched
#105 field-types-as-datatypes-testbot-1992138-105.patch270.74 KByched
#105 interdiff.txt8.01 KByched
#102 field-types-as-datatypes-testbot-1992138-102.patch267.64 KByched
#96 field-types-as-datatypes-testbot-1992138-96.patch267.94 KByched
#96 interdiff.txt3.09 KByched
#94 field-types-as-datatypes-testbot-1992138-94.patch265.56 KByched
#94 interdiff.txt22.26 KByched
#92 field-types-as-datatypes-testbot-1992138-92.patch250.7 KByched
#92 interdiff.txt745 bytesyched
#90 field-types-as-datatypes-testbot-1992138-89.patch252.43 KByched
#90 interdiff.txt24.67 KByched
#88 field-types-as-datatypes-testbot-1992138-88.patch246.68 KByched
#88 interdiff.txt12.91 KByched
#86 field-types-as-datatypes-testbot-1992138-86.patch243.04 KByched
#86 interdiff.txt12.91 KByched
#84 field-types-as-datatypes-testbot-1992138-83.patch238.9 KByched
#83 field-types-as-datatypes-testbot-1992138-83.patch0 bytesyched
#83 interdiff.txt6.96 KByched
#81 field-types-as-datatypes-testbot-1992138-81.patch238.43 KByched
#81 interdiff.txt8.07 KByched
#80 field-types-as-datatypes-testbot-1992138-80.patch239.91 KByched
#78 field-types-as-datatypes-testbot-1992138-78.patch240.9 KByched
#78 interdiff.txt4.86 KByched
#75 field-types-as-datatypes-testbot-1992138-75.patch234.7 KByched
#75 interdiff.txt2.73 KByched
#73 field-types-as-datatypes-testbot-1992138-73.patch234.06 KByched
#73 interdiff.txt10.03 KByched
#67 field-types-as-datatypes-testbot-1992138-67.patch224.58 KByched
#67 interdiff.txt5.48 KByched
#65 field-types-as-datatypes-testbot-1992138-65.patch223.95 KByched
#65 interdiff.txt15.81 KByched
#62 field-types-as-datatypes-testbot-1992138-62.patch209.16 KByched
#62 interdiff.txt1.89 KByched
#61 field-types-as-datatypes-testbot-1992138-61.patch209.02 KByched
#61 interdiff.txt907 bytesyched
#59 field-types-as-datatypes-testbot-1992138-59.patch208.86 KByched
#59 interdiff.txt9.3 KByched
#56 field-types-as-datatypes-testbot-1992138-56.patch205.71 KByched
#56 interdiff.txt71.24 KByched
#52 field-types-as-datatypes-testbot-1992138-52.patch183.72 KByched
#52 interdiff.txt828 bytesyched
#48 field-types-as-datatypes-testbot-1992138-48.patch182.91 KByched
#48 interdiff.txt6.23 KByched
#44 field-types-er-autocreate-1992138-44-interdiff.txt6.41 KBBerdir
#42 field-types-as-datatypes-testbot-1992138-42.patch174.66 KByched
#40 field-types-as-datatypes-testbot-1992138-40.patch174.66 KByched
#40 interdiff.txt11.07 KByched
#38 field-types-as-datatypes-testbot-1992138-38.patch164.49 KByched
#36 field-types-as-datatypes-testbot-1992138-36.patch177.52 KByched
#36 interdiff.txt3.07 KByched
#35 field-types-as-datatypes-testbot-1992138-35.patch174.76 KByched
#35 interdiff.txt1.58 KByched
#33 field-types-as-datatypes-testbot-1992138-33.patch169.16 KByched
#33 interdiff.txt2.23 KByched
#31 field-types-as-datatypes-testbot-1992138-31.patch168.52 KByched
#31 interdiff.txt4.94 KByched
#27 field-types-as-datatypes-testbot-1992138-27.patch169.9 KByched
#27 interdiff.txt1.42 KByched
#25 field-types-as-datatypes-testbot-1992138-25.patch169.65 KByched
#23 field-types-as-datatypes-testbot-1992138-23.patch169.25 KByched
#23 interdiff.txt930 bytesyched
#21 field-types-as-datatypes-testbot-1992138-21.patch168.22 KByched
#21 interdiff.txt686 bytesyched
#20 field-types-as-datatypes-testbot-1992138-19.patch168.24 KByched
#20 interdiff.txt720 bytesyched
#18 field-types-as-datatypes-testbot-1992138-17.patch167.8 KByched
#18 interdiff.txt1.96 KByched
#16 field-types-as-datatypes-testbot-1992138-15.patch166 KByched
#12 field-types-as-datatypes-testbot-1992138-12.patch165.67 KByched
#12 interdiff.txt557 bytesyched
#11 field-types-as-datatypes-testbot-1992138-11.patch165.69 KByched
#11 interdiff.txt10.16 KByched
#9 field-types-as-datatypes-testbot-1992138-9.patch160.65 KByched
#9 interdiff.txt7.52 KByched
#7 field-types-as-datatypes-testbot-1992138-7.patch155.45 KByched
#7 interdiff.txt9.63 KByched
#3 field-types-as-datatypes-testbot-1992138-3.patch146.46 KByched
#3 interdiff.txt9.09 KByched
#1 field-types-as-datatypes-testbot-1992138-1.patch139.99 KByched
#1 interdiff.txt1.22 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
1.22 KB
139.99 KB

Let's see where this brings us.
(interdiff is with @swentel's #1969728-28: Implement Field API "field types" as TypedData Plugins)

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-1.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
9.09 KB
146.46 KB

Some more fixes.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-3.patch, failed testing.

yched’s picture

Going after the FormTest fails tomorrow. Looks like the underlying code accounts for a fair part of the fails.

swentel’s picture

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

yched’s picture

Status: Needs work » Needs review
FileSize
9.63 KB
155.45 KB

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-7.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
7.52 KB
160.65 KB

Should fix fails in taxo/TermTest - EntityNG is *so* complicated :-/

+ commented out more tests on stuff that's not ported yet (validation, translation...)

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-9.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
10.16 KB
165.69 KB

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

yched’s picture

While we're at it, let's try this.

yched’s picture

Note: LinkItemTest fails will need a discussion with EntityNG folks in Portland, that's kind of intrinsically broken right now.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-12.patch, failed testing.

yched’s picture

Simple reroll for now.

yched’s picture

Status: Needs work » Needs review
FileSize
166 KB

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-15.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
167.8 KB

Let's see if this is better.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-17.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
720 bytes
168.24 KB

Can't work. Rather, this.

yched’s picture

Should fix a bunch of notices.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-21.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
930 bytes
169.25 KB

Should fix most upgrade fails. I hate you, update_module_enable().
(Basically, including hackish fixes from #1941000: update_module_enable() does not update ModuleHandler::$moduleList)

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-23.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
169.65 KB

Checking something.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-25.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
169.9 KB

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-27.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-27.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
4.94 KB
168.52 KB

Attempt at cleaning up some hacks.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-31.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.23 KB
169.16 KB

More fixes

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-33.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
174.76 KB

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

yched’s picture

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-36.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
164.49 KB

Checking the reroll.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-38.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
11.07 KB
174.66 KB

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-40.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
174.66 KB

Reroll.

amateescu’s picture

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

Berdir’s picture

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-42.patch, failed testing.

amateescu’s picture

Yep, that will fix the test, but the problem with empty $items in hook_field_presave() still remains..

Berdir’s picture

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

yched’s picture

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

yched’s picture

Status: Needs work » Needs review
amateescu’s picture

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

yched’s picture

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

yched’s picture

About 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]

yched’s picture

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

plach’s picture

I'll try to have a look to the failures during the weekend.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-52.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
71.24 KB
205.71 KB

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-56.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
yched’s picture

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-59.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
907 bytes
209.02 KB

Yet another stupid mistake.

yched’s picture

Getting there.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-62.patch, failed testing.

yched’s picture

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

yched’s picture

Status: Needs work » Needs review
FileSize
15.81 KB
223.95 KB

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-65.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
5.48 KB
224.58 KB

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

yched’s picture

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-67.patch, failed testing.

Berdir’s picture

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

plach’s picture

Sorry for being vacant. I will be able to review the field sync code tomorrow.

yched’s picture

Status: Needs review » Needs work

@plach: thanks !

From what I could tell with the latest patch, looking at this snippet in EntityTranslationSyncImageTest:

foreach ($entity->{$this->fieldName}[$default_langcode] as $delta => $item) {
  $value = $values[$default_langcode][$item['fid']];
  $source_item = $entity->{$this->fieldName}[$langcode][$delta];
  $assert = $item['fid'] == $source_item['fid'] && $item['alt'] == $value['alt'] && $item['title'] == $value['title'];
  $this->assertTrue($assert)

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

yched’s picture

Status: Needs work » Needs review
FileSize
10.03 KB
234.06 KB

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

The last submitted patch, field-types-as-datatypes-testbot-1992138-73.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
234.7 KB

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-75.patch, failed testing.

plach’s picture

After 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 and height 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.

yched’s picture

Status: Needs work » Needs review
FileSize
4.86 KB
240.9 KB

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-78.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
239.91 KB

Reroll.

yched’s picture

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-81.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
6.96 KB
0 bytes

Our legacy discovery cannot rely on HookDiscovery anymore, since this now fires Exceptions whithin upgrades.

yched’s picture

Doh, wrong git command.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-83.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
12.91 KB
243.04 KB

As discussed with @fago and @effulgentsia:
add the PrepareCacheInterface, and only call FieldItem::prepareCache() when needed.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-86.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
12.91 KB
246.68 KB

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-88.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
24.67 KB
252.43 KB

Forgot to remove the old testNestedFieldForm().
Interdiff with #86.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-89.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
745 bytes
250.7 KB

Code simplification.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-92.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
22.26 KB
265.56 KB

Fixed field purge, by making the $instance injectable at needed places in the callstack
Let's see if this breaks other stuff.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-94.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
3.09 KB
267.94 KB

Should fix the fails introduced above.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-96.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-96.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-96.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
267.64 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-102.patch, failed testing.

amateescu’s picture

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

yched’s picture

Status: Needs work » Needs review
FileSize
8.01 KB
270.74 KB

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

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-105.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
12.28 KB
268.69 KB

Fix issues with field purge (manipulating values of fields that do not officially exist)

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-106.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
641 bytes
265.62 KB

Reroll, + remove some test code that was not supposed to stay there.

yched’s picture

I can haz tests ?

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-109.patch, failed testing.

plach’s picture

I know I know, I am working on the field sync stuff tonight :)

yched’s picture

LOL

yched’s picture

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-114.patch, failed testing.

yched’s picture

Damn - need to run, will look into this later tonight

yched’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
267.42 KB

Partially reverts #114.
TypedDataInterface cannot extend PluginInspectionInterface, because Entity implements TypedDataInterface but cannot provide the PluginInspectionInterface methods.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-117.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
37.69 KB
280.31 KB

Rename a bunch of classes, using "C" as an abbrev. will never fly.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-119.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
yched’s picture

Checking where we stand with prepareView()
(expect lots of fails)

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-122.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
256.85 KB
yched’s picture

More hairpulling issues with the 'entity' property of taxo / entity_ref fields :-(
Checking something.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-125.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
3.55 KB
263.45 KB

back to checking prepareView() - field_view_field() ported.
(interdiff with #122)

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-127.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.78 KB
265.68 KB

Found a couple fixes.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-128.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
266.67 KB

Added missing BC support for invokeFieldMethodMultiple()

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-131.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
266.22 KB

Stupid mistake.

fago’s picture

some validation stuff.. -> testing

fago’s picture

yched: 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?

yched’s picture

@fago: you have commit access already, it seems ?

fago’s picture

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

yched’s picture

prepareView() / invokeFieldMethodMultiple: odd behavior in $entity->getTranslation()->get().
Silly workaround for now, let's see where this gets us.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-138.patch, failed testing.

yched’s picture

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

yched’s picture

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-testbot-1992138-141.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
20.31 KB
284.29 KB

ok, here is my complete interdiff (3commits).

    Fixed BC-mode letting 'entity' property of references vanish.
    Potentail fix for getTranslation() issue.
    Use getInstance() method to lazy-load and initialize field instances.
fago’s picture

Here is a variant containing only the first two commits (to see if new fails got introduced, where.)

    Potentail fix for getTranslation() issue.
    Use getInstance() method to lazy-load and initialize field instances.
fago’s picture

And lastly, here is just
Use getInstance() method to lazy-load and initialize field instances.

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#144: d8_field_types.patch queued for re-testing.

fago’s picture

ok, so this one changes #144 to keep catching exceptions.

Note, I've pushed this commits to field-types-as-datatypes-1969728-fago2

yched’s picture

Thanks 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]

fago’s picture

FileSize
301.47 KB

re-rolled patch and fixed some merge conflicts.

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

fago’s picture

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

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

fago’s picture

and this converts configurable field type manager to entity field type manager.

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#153: d8_field_types.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
302.68 KB

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

Status: Needs review » Needs work

The last submitted patch, d8_field_types-157.patch, failed testing.

amateescu’s picture

plus as part of that, changed TaxonomyTermReferenceItem to extend ConfigurableEntityReferenceItem (not sure if that's correct or not).

Not at all, we have #1847596: Remove Taxonomy term reference field in favor of Entity reference for that.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1006 bytes
303.48 KB

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

The last submitted patch, d8_field_types-160.patch, failed testing.

fago’s picture

Status: Needs review » Needs work
FileSize
305.62 KB
14.71 KB

TaxonomyTermReferenceItem to extend ConfigurableEntityReferenceItem (not sure if that's correct or not).

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

fago’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

amateescu’s picture

TaxonomyTermReferenceItem to extend ConfigurableEntityReferenceItem (not sure if that's correct or not).

oh, I think that's correct. I don't see how it would work else.

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

fago’s picture

I would honestly prefer to roll it back than to see this mess..

hm, 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!

amateescu’s picture

Sorry, 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.. :)

fago’s picture

No worries :)

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
426 bytes
305.99 KB

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

fago’s picture

Awesome - thanks!

As this is entity related I think it should be part of entity_info_cache_clear(), so moved it over there.

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
305.51 KB
624 bytes

damn - looks like the ordering is important here.

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

fago’s picture

FileSize
305.71 KB

ok, reverted my changes. So this is the Git branch + effulgentsia's interdiff.

fago’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#174: d8_field_types.patch queued for re-testing.

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -862,10 +862,11 @@ function translation_entity_field_info_alter(&$info) {
-  if ($entity->isTranslatable()) {
+function translation_entity_entity_presave(EntityInterface $entity) {
...
+  if ($entity->isTranslatable() && !empty($entity_info['fieldable'])) {

Not sure that fieldability should affect translation. This was removed for menu_link entity that none-fieldable by default but needs translation

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
305.47 KB

Straight reroll.

Status: Needs review » Needs work

The last submitted patch, d8_field_types-179.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
6.66 KB
304.79 KB

Status: Needs review » Needs work

The last submitted patch, d8_field_types-181.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
304.79 KB

Oh, the woes of copy/pasting.

effulgentsia’s picture

FileSize
304.77 KB

And even worse, rolling a patch incorrectly. Interdiff in #183 is correct, but here's the actual patch.

Status: Needs review » Needs work

The last submitted patch, d8_field_types-184.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
304.66 KB

Missed one.

msonnabaum’s picture

This looks like a bit of a performance regression :(

On a node page with 14 comments:

1992138-summary.png

The majority of it is in field_attach_prepare_view:

1992138-field_attach_prepare_view-summary.png

1992138-field_attach_prepare_view-calls.png

effulgentsia’s picture

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

effulgentsia’s picture

#186: d8_field_types-186.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, d8_field_types-186.patch, failed testing.

effulgentsia’s picture

I'm confused why #1969728-172: Implement Field API "field types" as TypedData Plugins is failing so much. Trying the same patch out here.

effulgentsia’s picture

Status: Needs work » Needs review
fago’s picture

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

fago’s picture

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

fago’s picture

Updated patch addressing reviews of #167-#169 except for re-naming classes (follow-up) and improving text-field item's code separation.

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#195: d8_field_types.patch queued for re-testing.

fago’s picture

ok, 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%.

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
314.09 KB
862 bytes

missed the picture test, with this one it's hopefully green again.

effulgentsia’s picture

From 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 !

effulgentsia’s picture

I'm a bit suprised that just removing the invokeFieldMethodMultiple('prepareView') call goes from -15% to +5% perf impact, somthing looks weird here.

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

fago’s picture

"In short: +1, awesome !"

Great to hear you like it.

Were those numbers generated with the 'features.comment_user_picture' theme setting on or off?

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.

I'm a bit suprised that just removing the invokeFieldMethodMultiple('prepareView') call goes from -15% to +5% perf impact, somthing looks weird here.

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.

fago’s picture

Status: Needs review » Closed (fixed)