Problem/Motivation

Despite its name, SelectionBase is in fact not a base class for the "entity_ref selection handler" plugins, but an actual implementation of a specific selection logic (restrict referenceable entities by bundle)

ViewsSelection provides a completely different logic (restrict referenceable entities by the results of a View), yet it extends SelectionBase, and has to undo all the parent selection logic to implement its own.

It seems this was mostly done to be able to share the code from validateAutocompleteInput::validateAutocompleteInput(), but that is generic code, agnostic about the actual selection logic, and could instead live in the Drupal\Core\Entity\Element\EntityAutocomplete FAPI element.

Proposed resolution

Patch :
- Moves SelectionBase to a new DefaultSelection class (the old SelectionBase is preserved as an empty class extending from it for BC, and is marked deprecated)
- Renames the associated plugin deriver to DefaultSelectionDeriver
- Makes ViewsSelection no longer extend SelectionBase
- Removes SelectionInterface::validateAutocompleteInput(), and move the former implementation in SelectionBase to EntityAutocomplete::matchEntityByTitle()

API changes

- One method is removed from SelectionInterface, which is not an API break. Potential existing implementations become dead code, but the method had no real value for being overriden anyway.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task: code cleanup, removing an inheritance between two plugin implementations that has no justification and is cumbersome, and renaming a "false" base class for clarity
Issue priority Normal
Disruption None
CommentFileSizeAuthor
#30 2578559-ER_SelectionBase-30.patch36.76 KByched
#25 2578559-ER_SelectionBase-25.patch22.28 KByched
#24 interdiff.txt722 bytesyched
#24 2578559-ER_SelectionBase-24.patch35.18 KByched
#21 interdiff.txt859 bytesyched
#21 2578559-ER_SelectionBase-21.patch35.18 KByched
#20 interdiff.txt644 bytesyched
#20 2578559-ER_SelectionBase-20.patch34.97 KByched
#10 interdiff.txt4.17 KByched
#10 2578559-ER_SelectionBase-10.patch34.34 KByched
#5 2578559-ER_SelectionBase-5.patch30.18 KByched
#3 interdiff.txt709 bytesyched
#3 2578559-ER_SelectionBase-2.patch30.08 KByched
#2 2578559-ER_SelectionBase-1.patch30.08 KByched

Comments

yched created an issue. See original summary.

yched’s picture

yched commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2578559-ER_SelectionBase-1.patch30.08 KB

Patch :
- Moves SelectionBase to a new DefaultSelection class (the old SelectionBase is preserved as an empty class extending from it for BC, and is marked deprecated)
- Renames the associated plugin deriver to DefaultSelectionDeriver
- Makes ViewsSelection no longer extend SelectionBase
- Removes SelectionInterface::validateAutocompleteInput(), and move the former implementation in SelectionBase to EntityAutocomplete::matchEntityByTitle()

yched’s picture

yched commented
StatusFileSize
new 2578559-ER_SelectionBase-2.patch30.08 KB
new interdiff.txt709 bytes

Meh, 3rd person in phpdoc

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: +VDC, +Needs beta evaluation

Tagging.

yched’s picture

yched commented
Issue tags: -VDC
StatusFileSize
new 2578559-ER_SelectionBase-5.patch30.18 KB

- facepalm on inconsistent rename between class & file
- I was being too smart with my "array_flattener", wanrings if the array is empty. array_walk() works, too.

@jibran : ViewsSelection is not "VDC", really, it's just some integration code for ER fields. It ships in views because it's meaningless if Views is disabled, but it's really ER code that the Views people most likely never even looked at :-)

Will add the beta eval.

yched’s picture

yched commented
Issue summary: View changes
Issue tags: -Needs beta evaluation

beta evaluation added

The last submitted patch, 2: 2578559-ER_SelectionBase-1.patch, failed testing.

The last submitted patch, 3: 2578559-ER_SelectionBase-2.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Thanks @yched.

yched’s picture

yched commented
StatusFileSize
new 2578559-ER_SelectionBase-10.patch34.34 KB
new interdiff.txt4.17 KB

Nice, green.

Last adjustment : Core classes that righfully extended SelectionBase are better off extending DefaultSelection rather than the now empty BC class.

amateescu’s picture

amateescu commented

The patch makes sense but are you sure this will be allowed to go in 8.0.x before RC?

Also, the issue summary doesn't mention that a method is removed from SelectionInterface, which is technically an API change...

yched’s picture

yched commented

Also, the issue summary doesn't mention that a method is removed from SelectionInterface [...]

Yes it does ;-)

[...] which is technically an API change

Hmm, API change but no API break ? Removing a method from an interface won't break any existing code, you less have conditions to fullfill to implement the interface - in that sense it's closer to an API addition ;-). Existing implementations just become dead code ?

amateescu’s picture

amateescu commented

Sorry I wasn't clear enough, it doesn't mention it in the 'API changes' block :P

Edit: and your reasoning sounds good, it doesn't break anything :)

yched’s picture

yched commented
Issue summary: View changes

Ah, true. Added.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Just need a change notice then it's RTBC.

yched’s picture

yched commented

CRs :
https://www.drupal.org/node/2578749
https://www.drupal.org/node/2578753

The last submitted patch, 2: 2578559-ER_SelectionBase-1.patch, failed testing.

The last submitted patch, 3: 2578559-ER_SelectionBase-2.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Thank @yched.

yched’s picture

yched commented
StatusFileSize
new 2578559-ER_SelectionBase-20.patch34.97 KB
new interdiff.txt644 bytes

Reroll after #2281533: Entity Reference default selection plugin ignores matches if an entity type has no label key, and moves the new PhpSelection to extend DefaultSelection rather than the deprecated SelectionBase.

yched’s picture

yched commented
StatusFileSize
new 2578559-ER_SelectionBase-21.patch35.18 KB
new interdiff.txt859 bytes

Oops, and adjusts the @see in PhpSelection accordingly.
Interdiff is with #10 for clarity.

The last submitted patch, 20: 2578559-ER_SelectionBase-20.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented 
+++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/PhpSelection.php
@@ -17,10 +17,9 @@
+ * @see \\Drupal\Core\Entity\Plugin\Derivative\DefaultSelectionDeriver

Nit: Double back-slash.

yched’s picture

yched commented
StatusFileSize
new 2578559-ER_SelectionBase-24.patch35.18 KB
new interdiff.txt722 bytes

Indeed :-)

yched’s picture

yched commented
StatusFileSize
new 2578559-ER_SelectionBase-25.patch22.28 KB

Reroll after #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2578559-ER_SelectionBase-25.patch, failed testing.

Status: Needs work » Needs review

yched queued 25: 2578559-ER_SelectionBase-25.patch for re-testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginManager.php
similarity index 89%
rename from core/lib/Drupal/Core/Entity/Plugin/Derivative/SelectionBase.php

rename from core/lib/Drupal/Core/Entity/Plugin/Derivative/SelectionBase.php
rename to core/lib/Drupal/Core/Entity/Plugin/Derivative/DefaultSelectionDeriver.php

Let's leave SelectionBase and gut it and deprecate it and make it extend DefaultSelectionDeriver to do a nicer BC compatible change.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new 2578559-ER_SelectionBase-30.patch36.76 KB

Damn. That was the plan, but it disappeared in the last reroll. Sorry about that.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

RTBC once again.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Okay so we have a BC layer and whilst I don't think this is super important for RC I think it'll be nicer to not live with this for the next x years. And we have less code to maintain. Committed 4ea2fc7 and pushed to 8.0.x. Thanks!

  • alexpott committed 4ea2fc7 on 8.0.x 
    Issue #2578559 by yched: Have ViewsSelection no longer extend...
amateescu’s picture

amateescu commented

Thanks, Alex! Published the CR.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.