Problem/Motivation

Many plugins, such as Blocks, need to translate strings when building a render array. Currently to get proper DI you have to e.g. use the ContainerFactoryPluginInterface and inject a translation object for every class.

Proposed resolution

Create a new class that is in the Core namespace that extends or replaces PluginBase and has available a $this->t()

Remaining tasks

TODO

User interface changes

None

API changes

Core plugins (e.g. BlockBase) that commonly need to translate strings can use the Core PluginBase

#2018411: Figure out a nice DX when working with injected translation

Follow Up Issues

#2170471: ContextAwarePluginBase compromised after commit of Core PluginBase

Original report by @pwolanin

Files: 
CommentFileSizeAuthor
#12 plugin-2087231-10.patch12.2 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,877 pass(es).
[ View ]
#12 interdiff.txt9.18 KBtim.plunkett
#4 plugin-2087231-3.patch3.02 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,585 pass(es).
[ View ]
#4 interdiff.txt2.2 KBtim.plunkett
#2 2087231-2.patch4.25 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,974 pass(es), 6 fail(s), and 2 exception(s).
[ View ]

Comments

Title:Extend PluginBase in the Core namespace like ControllerBase to is has a t() and any other common methodsExtend PluginBase in the Core namespace and like ControllerBase add to it a t() and any other common methods

fix title

Title:Extend PluginBase in the Core namespace and like ControllerBase add to it a t() and any other common methodsAdd a PluginBase in the Core namespace and like ControllerBase add to it a t() and any other common methods
Status:Active» Needs review
StatusFileSize
new4.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,974 pass(es), 6 fail(s), and 2 exception(s).
[ View ]

Given the truly minimal code in the Component class, it seemed cleaner to not inherit.

Here's a 1st stab, which copies the methods form FromBase and converts BlockBase.

Status:Needs review» Needs work

The last submitted patch, 2087231-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.2 KB
new3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 59,585 pass(es).
[ View ]

Discussed in IRC, let's use the base class.

Status:Needs review» Needs work

The last submitted patch, plugin-2087231-3.patch, failed testing.

Status:Needs work» Needs review

#4: plugin-2087231-3.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

we need one issue to do the same for DerivativeBase

i am rtbcing this, assuming we dont need to update all block plugins that use t() here

#4: plugin-2087231-3.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

+++ b/core/lib/Drupal/Core/Plugin/PluginBase.php
@@ -0,0 +1,61 @@
+use Drupal\Component\Plugin\PluginBase as ComponentPluginBase;

The one thing I took issue with is this and asked if we should instead rename that class to ComponentPluginBase, because it can be very confusing for there to be two classes called "PluginBase" (yes,as I realize namespaces differentiate them but that isn't always reflected in code editors).

Tim and James noted that if we were going to rename anything, it would make more sense to rename the core implementation (e.g. DrupalPluginBase or CorePluginBase) since the plugin system in Components is intended to be re-used outside of Drupal. Since we don't follow that pattern anywhere else (e.g. DrupalFormBase, which is ick), it doesn't make sense to introduce it here. If all of the implementations in core are deriving from the Core version, copy/paste developers will get the right one anyway.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.php
@@ -7,7 +7,7 @@
-use Drupal\Component\Plugin\PluginBase;
+use Drupal\Core\Plugin\PluginBase;

However, this is not currently happening, because we only have one of these hunks in the patch. So let's make sure everything is switched over to Core's plugin base in this patch, so we don't leave core in an inconsistent state.

Personally I don't think we should rename it. This is the only class that will interact with the two classes so it shouldn't matter. As stated if we where to rename a class the Core would be the one I'd vote for.

Sorry. I totally agree with not renaming it. That's a bad pattern to go down.

But I do think we need to make that same change we're making in BlockBase.php to all of the existing PluginBase-derived classes so that core is consistent with itself.

Status:Needs work» Needs review
StatusFileSize
new9.18 KB
new12.2 KB
PASSED: [[SimpleTest]]: [MySQL] 59,877 pass(es).
[ View ]

This should be all of them.

Title:Add a PluginBase in the Core namespace and like ControllerBase add to it a t() and any other common methodsAdd a PluginBase in the Core namespace with t() as a helper method

Retitled, we have no other obvious helpers to add at the moment, and getting all of the base plugins classes switched over will let us add anything more easily in the future.

Status:Needs review» Reviewed & tested by the community

Neat!

Title:Add a PluginBase in the Core namespace with t() as a helper methodChange notice: Add a PluginBase in the Core namespace with t() as a helper method
Priority:Normal» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record, +Approved API change

Committed and pushed to 8.x. Thanks!

Will need a change notice.

Issue tags:-Approved API change

Great patch !
Wouldn't url() / l() be helpful too ?

@yched - I'm less sure about that. Almost all plugins have some text many fewer generate link (though certainly some).

Issue summary:View changes

Updated issue summary.

Assigned:Unassigned» jgSnell
Issue summary:View changes

Working on the change notice now...

Title:Change notice: Add a PluginBase in the Core namespace with t() as a helper methodAdd a PluginBase in the Core namespace with t() as a helper method
Assigned:jgSnell» Unassigned
Priority:Major» Normal
Status:Active» Fixed
Issue tags:-Needs change record

Change notice is here: https://drupal.org/node/2116303

Issue summary:View changes

Status:Fixed» Closed (fixed)

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