Entity Reference Plugins have weird behaviour around their ID and other properties which make them a timesink when trying to create one.

This should be fixed -- see #2649712: Entity reference selection plugins break when not following a weird ID pattern (which also has several developers commenting on how they've wasted time figuring it out).

A fix will not be straightforward, so until that lands, the documentation should be improved so we stop wasting developers' time.

Things to decide here before a patch is worked on:

- where should the docs go: the plugin manager / the plugin base class / the plugin interface?
- what do the docs need to say?

To start with:

- An ERSP must declare its ID and group in one of the following forms:
-- group: GROUP, id: GROUP:ID (the id has the group as a prefix)
-- group: ID, id: ID (the id and the group are identical)
- the weight property is not used to order them in the UI, but rather to select the 'best' plugin within a group

CommentFileSizeAuthor
#26 2945789-ersp-26.patch1.46 KBjcisio
#19 2945789-19.patch1.04 KBravi.shankar
#16 2945789-16.patch1.54 KBravi.shankar
#12 2945789-ERS-plugin-quirks-12.patch1.03 KBjcisio
#11 2945789-ERS-plugin-quirks.patch1.03 KBjcisio

Comments

joachim created an issue. See original summary.

mpotter’s picture

mpotter commented

Oh yes ++ please. I spent hours banging my head trying to understand why my custom Selection plugin wasn't being discovered until I finally ran across this series of issues. The only example I could find in core was Views, which has the "group: views" and "ID: views" second form. But it never occurred to me the ID would matter.

Thanks for posting this!

joachim’s picture

joachim commented

@mpotter Is there anything you found that I've not listed in the summary?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Thanks for drawing attention to this!

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jcisio’s picture

jcisio commented

Where to document it? I don't see any page on drupal.org. Maybe update \Drupal\Core\Entity\Annotation\EntityReferenceSelection?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

If there is a documentation page on DO it should definitely be mentioned there too. But I agree that the annotation might be the best place to have something, as that's where most developers (the target audience for this issue) will look.

joachim’s picture

joachim commented

I am not sure where developers are most likely to look for guidance. My hunch is the interface, as the annotation is more out of the way in the file structure.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Why not both? :) Where one refers the reader to the other.

joachim’s picture

joachim commented

Yup, good idea!

jcisio’s picture

jcisio commented
Version: 8.6.x-dev » 8.7.x-dev
Status: Active » Needs review
StatusFileSize
new 2945789-ERS-plugin-quirks.patch1.03 KB

How about this? The annotation and the interface are cross referenced so there is no need to put it in both places. Only in the annotation there are comments about the properties.

jcisio’s picture

jcisio commented
StatusFileSize
new 2945789-ERS-plugin-quirks-12.patch1.03 KB

New patch with link to the actual issue, not to this one.

joachim’s picture

joachim commented
Status: Needs review » Needs work

The overall text is good.

> The annotation and the interface are cross referenced so there is no need to put it in both places.

I'm not seeing that cross-reference. Unless you mean just the basic cross-reference.

But it would be good for the interface to say something like "See the annotation for quirks concerning implementation of plugins of this type."

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Issue tags: +Novice
ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 2945789-16.patch1.54 KB

I have done this patch on Drupal-8.8.x-dev

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented 
+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityReferenceSelection.php
@@ -58,13 +65,15 @@ class EntityReferenceSelection extends Plugin {
-   * @var array (optional)
+   * @var arrayoptional

First time saw @var arrayoptional

joachim’s picture

joachim commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityReferenceSelection.php
@@ -7,7 +7,7 @@
- * Plugin Namespace: Plugin\EntityReferenceSelection
+ * Plugin Namespace: Plugin\EntityReferenceSelection.

I also wouldn't bother getting into the detail of whether this line needs a final full stop of not. That's not a fix related to this issue.

Apart from that and the comment in #17, this patch looks good! Nearly there!

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 2945789-19.patch1.04 KB
joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

Looks good. Thanks for the patch!

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2945789-19.patch, failed testing. View results

shashikant_chauhan’s picture

shashikant_chauhan commented
Status: Needs work » Needs review

This patch shouldn't fail as it's just updating the document.
Queueing the patch for retesting.

init90’s picture

init90
Location Ukraine
commented
Status: Needs review » Reviewed & tested by the community

Back to RBTC

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/Annotation/EntityReferenceSelection.php
@@ -23,6 +23,13 @@ class EntityReferenceSelection extends Plugin {
+   * @see https://www.drupal.org/project/drupal/issues/2649712

Either this shouldn't be here - it adds nothing to the current docs... or it should be an @todo to update these docs as a result of whatever is chosen to do in that issue. In order to not hold the much more important part of actually documenting this...

I was going to fix this on commit by removing the @see but then I saw the follwoing docs for \Drupal\Core\Entity\Annotation\EntityReferenceSelection::$group in the file:

   * @code
   * id = "node_advanced",
   * entity_types = {"node"},
   * group = "default",
   * weight = 5
   * @endcode

We need to update them so they actually work because according to these docs they won;t

jcisio’s picture

jcisio commented
Status: Needs work » Needs review
StatusFileSize
new 2945789-ersp-26.patch1.46 KB

Nice catch @alexpott.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 96dad7dc08 to 9.0.x and 24c3d0a2d6 to 8.9.x and 5ba7940278 to 8.8.x. Thanks!

I removed the @todo - it doesn't really add anything. If #2649712: Entity reference selection plugins break when not following a weird ID pattern changes how this works then it has to update the docs... that's just part of that issue. The @todo doesn't really add anything other than maintenance burden.

Backported to 8.8.x as a docs fix.

  • alexpott committed 96dad7d on 9.0.x 
    Issue #2945789 by jcisio, ravi.shankar, joachim, kristiaanvandeneynde,...

  • alexpott committed 24c3d0a on 8.9.x 
    Issue #2945789 by jcisio, ravi.shankar, joachim, kristiaanvandeneynde,...

  • alexpott committed 5ba7940 on 8.8.x 
    Issue #2945789 by jcisio, ravi.shankar, joachim, kristiaanvandeneynde,...

Status: Fixed » Closed (fixed)

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