Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

I don't see anything wrong with that, let's do it :)

pcambra’s picture

Status: Active » Needs review
FileSize
9.69 KB

Let's see what I broke

Status: Needs review » Needs work

The last submitted patch, 2017851-move_entity_reference_get_selection_handler-2.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
9.7 KB

Status: Needs review » Needs work

The last submitted patch, 2017851-move_entity_reference_get_selection_handler-4.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
835 bytes
9.7 KB

Let's see if this goes green, hopefully patch is aligned with the intention of the issue :)

amateescu’s picture

Status: Needs review » Needs work
+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/SelectionPluginManager.php
@@ -85,4 +85,26 @@ public function getSelectionGroups($entity_type) {
+   * @param $field
+   *   The field structure for the operation.
+   * @param $instance
+   *   The instance structure for $field on $entity's bundle.
+   * @param \Drupal\Core\Entity\EntityInterface|\Drupal\entity_reference\Plugin\Type\EntityInterface $entity
+   *   The entity for the operation.

$field and $instance are missing the 'array' data type, and $entity should be just \Drupal\Core\Entity\EntityInterface.

Otherwise we're good here :)

yched’s picture

They can still be used as arrays for now, but strictly speaking they are FieldInterface and FieldInstanceInterface objects. There are still old docs that refer to them as arrays here and there, but I'd suggest doc that gets added or moved around refers to the interfaces ?

amateescu’s picture

Yep, you're totally right.

pcambra’s picture

Status: Needs work » Needs review
FileSize
9.26 KB

Ok had big re-roll conflict issues so no interdiff on the doc changes :( but here is the patch!

amateescu’s picture

Title: Move entity_reference_get_selection_handler() to a method on SelectionPluginManager ? » Move entity_reference_get_selection_handler() to a method on SelectionPluginManager
Status: Needs review » Reviewed & tested by the community

Looks good to go :)

amateescu’s picture

Status: Reviewed & tested by the community » Needs work
Pancho’s picture

Status: Needs work » Needs review
FileSize
9.26 KB

Here's the reroll. Should still be RTBC, when green.

pcambra’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as for #12

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Should we actually be removing the function entity_reference_get_selection_handler() as well? It was a one point but in #10 it was no longer removed... can't determine why... as this patch does appear to remove all 10 usages from core.

Also needs a reroll..

curl https://drupal.org/files/2017851-move_entity_reference_get_selection_handler-13.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  9486  100  9486    0     0   9800      0 --:--:-- --:--:-- --:--:-- 12038
error: patch failed: core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/SelectionPluginManager.php:10
error: core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/SelectionPluginManager.php: patch does not apply
amateescu’s picture

Issue tags: -Needs reroll

Yes, we should. I guess it was just a victim of the big conflict reroll from #10..

amateescu’s picture

Status: Needs work » Needs review
FileSize
9.91 KB

Rerolled and deleted the deprecated function.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC since the patch is really trivial :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Type/SelectionPluginManager.phpundefined
@@ -91,4 +93,24 @@ public function getSelectionGroups($entity_type) {
+    return \Drupal::service('plugin.manager.entity_reference.selection')->getInstance($options);

I might be missing something... but shouldn't we using $this->getInstance($options); here?

amateescu’s picture

Status: Needs work » Needs review
FileSize
779 bytes
9.85 KB

Of course we should, me--. Got to respect that self-imposed break more :)

Status: Needs review » Needs work
Issue tags: -RTBC July 1

The last submitted patch, 2017851-move_entity_reference_get_selection_handler-21.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: +RTBC July 1
tim.plunkett’s picture

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceAutocomplete.phpundefined
@@ -70,7 +70,7 @@ public function getMatches($field, $instance, $entity_type, $entity_id = '', $pr
-    $handler = entity_reference_get_selection_handler($instance, $entity);
+    $handler = \Drupal::service('plugin.manager.entity_reference.selection')->getSelectionHandler($instance, $entity);

EntityReferenceAutocomplete is a service, so this can be easily injected

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteTagsWidget.phpundefined
@@ -39,7 +39,7 @@ class AutocompleteTagsWidget extends AutocompleteWidgetBase {
+    $handler = \Drupal::service('plugin.manager.entity_reference.selection')->getSelectionHandler($this->fieldDefinition);

We really need to fix/kill WidgetFactory...

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceSelectionSortTest.phpundefined
@@ -121,7 +121,7 @@ public function testSort() {
+    $handler = \Drupal::service('plugin.manager.entity_reference.selection')->getSelectionHandler($instance);

@@ -137,7 +137,7 @@ public function testSort() {
+    $handler = \Drupal::service('plugin.manager.entity_reference.selection')->getSelectionHandler($instance);

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/Views/SelectionTest.phpundefined
@@ -70,7 +70,7 @@ public function testSelectionHandler() {
+    $handler = \Drupal::service('plugin.manager.entity_reference.selection')->getSelectionHandler($instance);

This could be $this->container->get(...

amateescu’s picture

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

Can we get an issue for WidgetFactory to extend or be replaced by ContainerFactory? Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs followup

Committed 42ebadc and pushed to 8.x. Thanks!

Created followup #2052751: WidgetFactory is not used anywhere so it should be removed

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