Some e_r fields get the wrong Selection handler

Patch, let's see what the bot says.

Migrate test needs adjustment

Ooh, right, tough one, #2436835: Unable to create config schema for entity type specific entity reference selection plugin. :-/

Hm - looks like a few configurable fields shipped in standard.profile have wonky selection plugins :
field.field.node.article.field_image.yml : default:node
field.field.user.user.user_picture.yml : default:node

@amateescu : we should change those to default:file, right ?
I guess that means an update function as well :-/

tagging accordingly then

@amateescu : Hm - but we're not supposed to call APIs in update funcs, so we can't really rely on $selection_manager->getPluginId() as entity_reference_field_config_presave() does ?

Not sure what the logic should be in that update, actually :-)

Also, entity_reference_field_config_presave() :
- is actually not doing enough, since it only cares about fields of type 'entity_reference', while the handler needs to be adjusted for "child" field types as well (file, image...)
- should be in field_module rather than entity_ref.module, since this affects fields that can be created without e_r.module
- entity_reference_field_storage_config_presave() should have the same treatment, for consistency

Updated patch moves them to field.module, and makes them handle child types.

Haven't really figured out the question of the update for currently faulty fields.

Actually, for the update, a hook_post_update() that just resaves the field definitions should work. Added a test as well.

Hopefully this should be committable.

Updated the IS and added a beta evaluation

Of course it would help if I actually fixed the broken yamls in standard.module...

This should be green.

[edit : removed uninteresting debug rambling, fail was just me being über-silly]

Oh gee. I had left two copies of the logic in that resets 'handler_settings' in field_field_storage_config_update().

/me slams his head against the wall, and goes out for a walk

This should really be green now...

Indeed, I wasn't aware of the [module].post_update.php convention. Thanks !

Yeah, not my day.

Oh, and UpdatePostUpdateTest, you suck.

Not sure what to believe between https://qa.drupal.org/pifr/test/1169638 reporting fails and https://dispatcher.drupalci.org/job/default/19890/testReport/Installer/D... reporting passes.

Passes locally, so, retest ?

@jibran #35 :

I think we should add some custom fields in update test to check that it is working for custom fields(not defined by config files)

I don't see the point. Fields shipped in config are just regular configurable fields, no different from the ones created in the UI. The test tests a configurable field that's present in drupal-8.bare.standard.php.gz, whether it came from the install profile or was added later on makes no difference.

as well and some asserts in EntityUnitTest after saving the ER field config instance so that we can make sure it'll never happen again as in set default handler and assert that it stores default:reference_entity_type

That is actually already tested in EntityReferenceItemTest::testSelectionHandlerSettings(), because entity_reference_field_config_presave() took care of it.

The thing that was broken was that entity_reference_field_config_presave() didn't take care of sub field types (file, image), and field_field_config_presave() now does. Added a test for that in FileItemTest (fails on HEAD)

Also, added a comment for the correct but non-explicit flow for "reassign the correct handler plugin in the fields when the target_type changes in the field storage". Added more explicit tests in EntityReferenceItemTest::testSelectionHandlerSettings() (they pass on HEAD)

#35.1 :-)
#35.2 Added a comment
#35.3 .4 :-)

@xjm : I think rc target is fine. This breaks no APIs and can happen after RC1, but would be really best to get in before RC1 because the current behavior (e.g. field_image using the NodeSelection plugin) is very weird and can lead to nasty consequences.

Test-only patch fails, patch passes...

RTBC anyone ? ;-)

Reroll

Reroll after #2571533: Allow setting custom storage on FieldStorageConfig, that also added a field_post_update

As mentioned by @alexpott on IRC, #48 was too big, diffed against the wrong HEAD, I guess.

Re-rebased, sizes match.

Reroll needed after #2520540: Enforced configuration dependencies shouldn't have to be repeated in the calculated dependencies, that had to adjust the list of post_updates in UpdatePostUpdateTest as well.

Not where I can do this atm, anyone up for it ? ;-)

Reroll

Bot fluke, back to green

@larowlan : Yes, there's kind of an expectation that if a field type extends another one, it has all the settings of the parent. Otherwise things might break any time the parent class adds some code using $settings['the_setting'] in a point release ?