Suggested commit message:

Allow plugins to declare themselves a derivative #2184951 by not_chx

please do not credit chx, wrong browser for the reroll.

Problem/Motivation

The plugin derivative system works well in most cases: if you want to display a menu block, one class can handle all the menus. One menu is like the next.

For migration destination plugins this, however, does not work because it needs to work with various entity types and one entity type is definitely not like the other. There needs to derivatives for every entity type but determining which class serves which type is slightly problematic. Some entity types have specific classes, most, however can be served by the content or the config base, depending. However, when defining the derivatives discovering the specific classes is a problem.

Proposed resolution

A small change in the DerivativeDiscoveryDecorator allows plugins to declare themselves as derivatives. In other words @PluginID('entity:comment). Instead of solving the problem of the DerivativeDiscoveryDecorator finding EntityComment, we rely on the annotated discovery already finding it. In theory this was agreed on by dawehner

I agree that a plugin should be able to specify a plugin ID which is not overridden by a derivative class

Remaining tasks

None.

User interface changes

None.

API changes

None. This is an addition.

Major as it's a migrate blocker.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

not_chx’s picture

Issue summary: View changes
not_chx’s picture

Issue summary: View changes
not_chx’s picture

FileSize
1.42 KB

More straightforward patch.

not_chx’s picture

Status: Active » Needs review
FileSize
2.25 KB

With more comments and more readable code flow than the original (which conflated plugin_definition and base_plugin_definition)

not_chx’s picture

Priority: Normal » Major
Issue summary: View changes
not_chx’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 4: 2184951_4.patch, failed testing.

not_chx’s picture

FileSize
2.23 KB
not_chx’s picture

Status: Needs work » Needs review
not_chx’s picture

Issue summary: View changes
FileSize
5.11 KB
not_chx’s picture

FileSize
5.22 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It would have been kinda cool if the other instances of the pattern (as you know views) would have been converted
so we see it also works in "real life" examples, though this does not block this issue to be RTBC.

not_chx’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.61 KB

Oh sure, let's convert Views!

not_chx’s picture

Status: Needs review » Reviewed & tested by the community

I will file a followup instead -- turns out my conversion misses all the keys ViewsEntityRow::getDerivativeDefinitions does.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2184951_13.patch, failed testing.

not_chx’s picture

Status: Needs work » Reviewed & tested by the community

#11 is still RTBC.

tim.plunkett’s picture

This seems to be only useful for @PluginId, which is not many things. If we can't get that Views usecase working, I'm not sure it should go in alone...

not_chx’s picture

Oh we absolutely can we make it work, it's just more work -- I need to copy every key and I only copied one.

not_chx’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
9.93 KB

Here's all three. I ran the node RowPluginTest and it passed.

Status: Needs review » Needs work

The last submitted patch, 19: 2184951_11.patch, failed testing.

not_chx’s picture

Status: Needs work » Needs review
FileSize
9.08 KB

Here's a version that uses derivative settings set by the deriver as default -- ie the plugin definition merely overrides much like the alter did.

dawehner’s picture

interdiff *sniff* *sniff* Note: if you have a couple of minutes to review a patch during breakfast you don't wanna start from 0 again

dawehner’s picture

Beside from that it is looking great.

not_chx’s picture

Sorry but the interdiff is 9kb and I doubt that would've helped reviewing a 9kb patch...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

tim.plunkett’s picture

+++ b/core/modules/comment/comment.views.inc
@@ -676,15 +676,3 @@ function comment_views_data_alter(&$data) {
-
-/**
- * Implements hook_views_plugins_row_alter().
- *
- * Replaces the generic row plugin by a custom one for comments.
- *
- * @see \Drupal\views\Plugin\views\row\EntityRow
- */
-function comment_views_plugins_row_alter(array &$plugins) {
-  $plugins['entity:comment']['class'] = 'Drupal\comment\Plugin\views\row\CommentRow';
-  $plugins['entity:comment']['provider'] = 'comment';
-}

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/row/CommentRow.php
@@ -11,6 +11,10 @@
+ *
+ * @ViewsRow(
+ *   id = "entity:comment",
+ * )

Yes please! Keeping all of the metadata in the file instead of in a procedural alter is awesome.

RTBC +1

not_chx’s picture

Note that a change notice IMO is not necessary -- this is an addition; the old alter hook still works if anyone happened to use it and there is no change notice talking about defining derivatives anyways. Also, this is such an edge case that I seriously doubt anyone already uses it. Consider how similar Views and migrate are in breadth: they need to cover for every module in core, they have "dynamic" plugin managers (multiple types in one class) and so on -- these are really atypical of a contrib and even more so of a custom module. I do not expect the change in this issue to be used much outside of these two modules.

There's a handbook page https://drupal.org/node/1653226 or two https://drupal.org/node/1653532 which is relevant but they are hopelessly outdated and need to rewritten from ground up anyways.

chx’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
461 bytes
9.08 KB

Keeping up with HEAD. interdiff would be meaningless so I attached a diff of the diffs: the patch didn't change at all except in context and an empty line removal.

not_chx’s picture

Issue summary: View changes
not_chx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, no changes, was NR only for bot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 2184951_28.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review

28: 2184951_28.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 28: 2184951_28.patch, failed testing.

The last submitted patch, 28: 2184951_28.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

28: 2184951_28.patch queued for re-testing.

not_chx’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yeah, the code snippet in #26 indeed looks much nicer. The work done in getDefinition() also makes that function more readable as a nice bonus.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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