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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

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

fix title

pwolanin’s picture

Title: Extend PluginBase in the Core namespace and like ControllerBase add to it a t() and any other common methods » Add a PluginBase in the Core namespace and like ControllerBase add to it a t() and any other common methods
Status: Active » Needs review
FileSize
4.25 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
3.02 KB

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

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review

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

ParisLiakos’s picture

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

pwolanin’s picture

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

webchick’s picture

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.

neclimdul’s picture

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.

webchick’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
9.18 KB
12.2 KB

This should be all of them.

tim.plunkett’s picture

Title: Add a PluginBase in the Core namespace and like ControllerBase add to it a t() and any other common methods » Add 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.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Neat!

webchick’s picture

Title: Add a PluginBase in the Core namespace with t() as a helper method » Change 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.

yched’s picture

Issue tags: -Approved API change

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

pwolanin’s picture

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

tim.plunkett’s picture

dawehner’s picture

dawehner’s picture

Issue summary: View changes

Updated issue summary.

jgSnell’s picture

Assigned: Unassigned » jgSnell
Issue summary: View changes

Working on the change notice now...

jgSnell’s picture

Title: Change notice: Add a PluginBase in the Core namespace with t() as a helper method » Add 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

EclipseGc’s picture

Issue summary: View changes

Status: Fixed » Closed (fixed)

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