make the 'group' property on EntityReferenceSelection plugins implicitly derived

Can we still reach out to whoever wrote this to hear why the whole concept of groups was introduced to begin with? I feel like another solution might be to just drop the whole idea of groups.

In theory we could do that. The field settings would instead let you pick all the plugins that declare they work with the current entity type.

But in the SelectionPluginManager there's a lot of logic for selecting the 'best' plugin, so of which seems irrelevant given what EntityReferenceItem does, but which could be in use in the wild. In particular, someone could have added a default:node_extra plugin and relied in the weight to get that selected automatically in a custom form that uses the entity_autocomplete element. There'd be a lot of behaviour to first DOCUMENT (!!!!) then write tests for (there are none!! SURPRISE!!!) and then deprecate with BC. I'm not against doing that, but the MR here is a nice quick solution which means no more headdesking for devs trying to write a plugin.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Something not mentioned in the summary is that there can be more than one colon. The ID could be foo:bar:baz

I'm pretty sure that when it comes to selection handlers, there can be multiple colons.

True, but that won't make any difference. It's the base plugin ID that has to match the group.

Appears there are number of test failures.

I was getting a different test failure local (?!!!) but I think this will fix both.

Still appears to be failing

Sorry took me so long to get back to this one.

AFAICT the only failures are this deprecation:

> Setting the 'group' property on selection plugin 'default:entity_test_with_bundle is deprecated in drupal:11.1.0 and will be removed in drupal:12.0.0. The property should be removed. See

(Is there a way of finding test failures on gitlab? I'm having to scroll through endless pages of test result trying to look for them. Is this actually what we're meant to do?)

But I can't reproduce locally, e.g. modules/field/tests/src/Kernel/Entity/FieldConfigValidationTest.php --filter=testEntityReferenceSelectionHandlerIsValidated runs fine for me.

My branch is rebased, so going to push that and see what gitlab says.

> But I can't reproduce locally, e.g. modules/field/tests/src/Kernel/Entity/FieldConfigValidationTest.php --filter=testEntityReferenceSelectionHandlerIsValidated runs fine for me.

My phpunit.xml needed to be updated for PHPUnit 10.5.

Fixed the test failures, I think.

Cleaned up IS some.

Changes look good, think ready to add a CR

Done.

Hi,

I applied the patch successfully and created the custom 'EntityReferenceSelection' plugin.

If the 'plugin_id' is "mymodule_node_type_selection:test", I did not specify any group name, so it is dynamically set to "mymodule_node_type_selection".

If the 'plugin_id' is “mymodule_node_type_selection”, the group name matches the 'plugin_id'.

So RTBC ++

I would greatly appreciate any suggestions or if there’s anything I might have missed in testing.

Thank you!

@sahana_n adding a screenshot of your console of a patch applying is no needed, that's evident by the MR being mergable.

Left 2 small comments on the MR, but looks super close!

> Could we get a simple test case for the deprecation test.

Do we really have to?

Mocking plugin discovery is really painful -- see ReflectionFactoryTest and MigrateFieldPluginManagerTest for instance. It's a LOT of test code to write just for a deprecation.

Addressed the other points.

Can see what other reviewers think. Thanks for the other points

Been a few weeks and I could wrong about the need for a deprecation test. But will see what core committers think.

I filed ✨ make it easier to mock plugin discovery in tests Active BTW.

So different case that deprecation test could be added to that ticket as an example. I'd be cool with that too.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I've checked the issue and identified an item related to the processDefinition method's return type. I've added the necessary type hint and am now moving this to Needs Review, Please review.