Due to the initiative to create separate dedicated Annotation (#1966246: [meta] Introduce specific annotations for each plugin type)
the question came up: How do we document that a certain key needs a wrapped @Translation.

Some suggestions:

1)

  /**
   * The description of the plugin.
   *
   * The string should be wrapped in a @Translatable()
   *
   * @var string
   */
  public $description;

2)

  /**
   * The description of the plugin.
   *
   * @see \Drupal\Core\Annotation\Translation
   *
   * @var string
   */
  public $description;

3)

  /**
   * The description of the plugin.
   *
   * @var \Drupal\Core\Annotation\Translation
   */
  public $description;

4)

  /**
   * The description of the plugin.
   *
   * @var \Drupal\Core\Annotation\Translation|string
   */
  public $description;
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Personally I would go with @var ... because it makes it as clear as possible.

tim.plunkett’s picture

Issue summary: View changes

numbering the options

tim.plunkett’s picture

I vote for anything but #1.
I think #2 is the most conservative option

#3 is what we want people to do, but I'm not sure if its more confusing.

#4 is technically correct, because Drupal will accept either, even though we want it to be translated.

EclipseGc’s picture

I vote for 3.

Eclipse

webchick’s picture

Component: plugin system » documentation

Moving to documentation so jhodgdon can take a look.

jhodgdon’s picture

Well, I don't really understand this issue so it is hard to comment... What does it mean "a certain key needs a wrapped @Translation."? What does a developer need to do differently between "keys" (whatever those are) that do and don't need a "wrapped @Translation"? And where are you proposing putting this documentation (on the base class or something?)

webchick’s picture

Right.

So in Drupal 8, there are these "plugins" that are defined by "annotations" (special array syntax in PHP comments) like:

/**
 * Provides a 'Book navigation' block.
 *
 * @Plugin(
 *   id = "book_navigation",
 *   admin_label = @Translation("Book navigation"),
 *   module = "book"
 * )
 */
class BookNavigationBlock extends BlockBase {

These effectively replace "info" hooks, and aim to describe the thing they're defining right above the thing, rather than in a different place.

@Translation() effectively works like t() and marks something as a human-readable label that is output in the admin interface.

Many developers pushed back (rightly so) on these annotations because there was absolutely no way to know what possible keys could go into one without getting neck-deep in the code.

So, we've started a movement where we create effectively "typed" Plugins, like for example @EntityType instead of @Plugin, which can be thoroughly documented as to what they include.

This creates a class like the following:

+class EntityType extends Plugin {
+
+  /**
+   * The name of the module providing the type.
+   *
+   * @var string
+   */
+  public $module;
+
+  /**
+   * The name of the entity type class.
+   *
+   * This is not provided manually, it will be added by the discovery mechanism.
+   *
+   * @var string
+   */
+  public $class;
+
...
+  /**
+   * The human-readable name of the type.
+   *
+   * @var string
+   */
+  public $label;
+

So the question is, "In the documentation for EntityType, how do we differentiate between a human-readable string that must be wrapped in @Translation() in the annotation syntax, vs. just a normal everyday string?"

Does that make sense at all?

jhodgdon’s picture

Thanks for the explanation!

I've always favored adding the words/phrase "human-readable" or "translated" in function/hook docs for parameters and return values that should be translated.... but this is a bit different...

So... My proposal would be that we try just being explicit:

  /**
   * The human-readable name of the type.
   *
   * In your plugin annotation, this value must be wrapped in @Translation().
   *
   * @var string
   */
  public $label;

And by the way... We have some documentation on plugin annotation using @Plugin and also the API module has been adapted to handle this specific annotation (it basically treats it as a @code/@endcode block). Are you saying now that it could be @BlockEntity, @SomeOtherClass, etc. rather than literally @Plugin? If so, how is the API module supposed to differentiate between this and other tags? Any ideas? (and should this go on another issue?)

webchick’s picture

Hm. That should probably go in another issue, probably in the API queue. That's a head-scratcher, for sure. :\ Maybe trigger off whether it's @CapsCaps instead of @lowercase?

dawehner’s picture

I guess the API module could/should understand the concept of plugins and use doctrine, which gives us this annotation syntax, to scan these things?

tim.plunkett’s picture

Title: How do document needed @Translation on dedicated » How do document needed @Translation on dedicated annotation classes
jhodgdon’s picture

Title: How do document needed @Translation on dedicated annotation classes » How do document needed @Translation on dedicated

We also need to update the standards page on plugin annotations, so I filed
#1970900: Update standards doc on plugin annotations; add @defgroup annotation
in Drupal Core for the moment; will file separate issue in the API module once I know what it should do. :)

So back to this issue... thoughts on #17's idea for this topic?

EclipseGc’s picture

Well, I think all of these top level classes that are replacing @Plugin are going to be extending it (if that's helpful to api.module).

jhodgdon’s picture

Title: How do document needed @Translation on dedicated » How do document needed @Translation on dedicated annotation classes

cross-post, restore title

jhodgdon’s picture

Let's move the discussion about the @Plugin stuff to the other issue.

dawehner’s picture

Status: Active » Needs review
FileSize
779 bytes

Here is a patch to document this in EntityType().

jhodgdon’s picture

+1 for #15. I think it's clear. Is it too much of a burden on core developers to do this?

jhodgdon’s picture

Oh... But I think we also need to have the @Translation() escaped in some way or the API module could be confused into thinking this is plugin annotation if the doc block ended in a ) for some reason such as an @see tag.... So either put a space between the @ and Translation, or prefix @ by a backslash. This will not help clarity. :(

EclipseGc’s picture

I don't see this as being particularly onerous.

webchick’s picture

Well, it's a lot of text. And it's text we'd want to make consistent across probably 500 properties. That's why I'd prefer something more like #3 in the OP which is easier to copy/paste.

EclipseGc’s picture

I preferred 3 as well, and it fixes the issue in #17

jhodgdon’s picture

So... Are these classes you are defining real classes that store real values on them? And if they are, is (for example) the $label property in EntityType actually an instantiated value of type Translation, or is it a plain-old PHP string value that is somehow magically translated with t()?

If all that is correct, then I agree that putting @var \Drupal\Core\Annotation\Translation makes sense on the EntityType class's $label property (option #3 in the issue report currently), and hopefully someone will also document somewhere that this means you need to put @Translation() in your @EntityType annotation too.

webchick’s picture

I don't know the answer to #21 (I suspect not), but another thought is just a simple "@ingroup Translatable" ... that's how we do some similar things elsewhere.

jhodgdon’s picture

+1 on that idea (@ingroup something), assuming we add some documentation to the @defgroup.

EclipseGc’s picture

They should be the actual class as a value and then when @EntityType::get() is called, the Translation::get() is called on that property and the translated value for the string is returned to the @EntityType class's representation of the annotation which is ultimately an array.

Eclipse

jhodgdon’s picture

So... #24 didn't actually make sense to me.

But let's go back to #22. Can we just make a defgroup that explains the need to put @Translation() in the annotation for certain of your plugin properties?

And ... oops, probably a separate issue, but are the plugin extension classes themselves part of a group/topic as well? If not, can we make them be so they are discoverable by developers, the same way as we have @ingroup hooks and @ingroup themeable?

EclipseGc’s picture

I have no idea what you're asking in your last paragraph of 25. That being said, if I understood #22, I don't have any issue with the proposal.

Eclipse

tim.plunkett’s picture

David_Rothstein’s picture

Several people at #1966246: [meta] Introduce specific annotations for each plugin type seem to believe that issue could result in @Translation() no longer being necessary...

I don't know if that will be the case or not, but it might not be worth expending too much effort here until that discussion is settled?

jhodgdon’s picture

It looks like the last comment there (just after #28 here was posted) pointed out we still need @Translation() so that localize.drupal.org can pick out the strings that need to be translated, or at least that it makes localization string identification much simpler if it's there.

jhodgdon’s picture

Title: How do document needed @Translation on dedicated annotation classes » How to document needed @Translation on dedicated annotation classes
EclipseGc’s picture

Yup, I think we should continue forward on this issue.

jhodgdon’s picture

Status: Needs review » Needs work

OK, I think we've all agreed that the previous patch is not all that workable. Can we get a patch that:
a) Adds a @defgroup translatable that describes this phenomenon
b) Adds @ingroup translatable to some translatable strings on EntityType

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.66 KB

So here is a start.

jhodgdon’s picture

Status: Needs review » Needs work

You cannot put a @defgroup inside of Class documentation. It needs to be in its own doc block by itself. Putting it into the file for the Translation class does seem like a good idea though. Here's the doc on how to write @defgroups:
http://drupal.org/coding-standards/docs#defgroup
Hm. Maybe that isn't clearly stated there... I will edit the page so it is.

Also, keep in mind that the Topic page will have the @defgroup text at the top, and a list of everything marked as being in the group below it. So saying "this class" won't work (you won't be able to tell which of the items listed below on the topic page it refers to). So, that documentation needs to be rewritten.

I also think that the topic text (the @defgroup documentation) should describe how to make a property translatable with annotation.

Finally, in the actual properties, the @ingroup should come at the end of the doc block.
http://drupal.org/coding-standards/docs#order

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.86 KB

So what about something more in the direction of that?

I switched the documentation group, because it just feels wrong to block such a generic term with plugin annotation translations.

jhodgdon’s picture

Status: Needs review » Needs work

Better... One note: if you are using @something() in a doc block as documentation, you need to leave a space between the @ and the something, or the doc parser will get confused and think it is annotation.

And can I offer a suggested docs edit (which may still some work):

When providing plugin annotation, properties whose values are displayed in the user interface should be made translatable. Much the same as how user interface text elsewhere is wrapped in t() to make it translatable, in plugin annotation, wrap translatable strings in the @ Translation() annotation. For example:
@code
title = @ Translation("Title of the plugin"),
@endcode
You will also need to make sure that your class file includes the line
@code
use Translation;
@endcode

Also, can we make the defgroup machine name a bit shorter? How about plugin_translatable?

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
1.99 KB

The reason why it doesn't fail is because it's never scanned.
By default only files in the Plugins directories are scanned, so we are on the safe site.

jhodgdon’s picture

I was talking about the API module that scans docs for api.drupal.org. I am pretty sure that it will get confused if you leave @Translation() in the docs without putting in a space. I think we need to have a space inside @code as well to avoid this problem, and probably we should put a note in the text about removing the space. We did something similar elsewhere... but I'm not remembering where (or perhaps it was a previous patch on this same issue?).

dawehner’s picture

Category: task » feature
dawehner’s picture

Category: feature » task

.

jhodgdon’s picture

Ah thanks. Yes, here's the resulting api.drupal.org page from that other issue, showing the spaces after @:
PagerPluginBase class

dawehner’s picture

FileSize
758 bytes
2.12 KB

So let's remove the space, that the api module is not confused.

jhodgdon’s picture

Status: Needs review » Needs work

This looks good to me, except that the @defgroup and @ingroup machine names do not agree in the Translation class itself. Oops! :)

So, I basically wrote the current docs test... dawehner do you agree that it's accurate, complete, etc. so we can call it duly reviewed? I of course think it's fine. :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
2.11 KB

Oops.

The text is certainly nice to read, so I hope it's understandable for everyone. Primarily I just want to get rid of this blocker.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

OK, then, since two of us like it, let's get it committed. I'll get to it shorltly unless another committer beats me to it. Thanks!

dawehner’s picture

Awesome. Thank you very much. This unblocks a ton of other issues.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

This is committed to 8.x. Thanks all!

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Status: Closed (fixed) » Needs work
+++ b/core/lib/Drupal/Core/Annotation/Translation.php
@@ -10,6 +10,28 @@
 /**
+ * @defgroup plugin_translatable Translatable plugin metadata
+ *
+ * @{
+ * When providing plugin annotation, properties whose values are displayed in
+ * the user interface should be made translatable. Much the same as how user
+ * interface text elsewhere is wrapped in t() to make it translatable, in plugin
+ * annotation, wrap translatable strings in the @ Translation() annotation.
+ * For example:
+ * @code
+ *   title = @ Translation("Title of the plugin"),
+ * @endcode
+ * Remove spaces after @ in your actual plugin - these are put into this sample
+ * code so that it is not recognized as annotation.
+ *
+ * You will also need to make sure that your class file includes the line:
+ * @code
+ *   use Drupal\Core\Annotation\Translation;
+ * @endcode
+ * @}
+ */
+

This did not document the possibility to include context information in the annotation. Code like http://drupalcode.org/project/drupal.git/blob/HEAD:/core/lib/Drupal/Core... uses that and since this was not considered when the D8 API compatibility code was written for localize.drupal.org, those strings are now extracted as Bundle", context = "Validation Yeah :/

It would be great to document context here and provide an example.

jhodgdon’s picture

Gabor: I think we should open a new issue for this rather than trying to reopen this one? I am not sure what you are asking for either -- seems like you are also reporting a bug?

Gábor Hojtsy’s picture

The documentation added here was incomplete / misleading. The bug with parsing is already fixed in potx module.

jhodgdon’s picture

OK... I'm unsure what needs to be done then.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.03 KB

The first part of the docs explains @Translation as a limited thing (no context), while the second part of the docs explains @Translation as more powerful than it is (with full support for t() stuff). So both need fixing. A proposal may be telling better what I'm talking about?! What about this?

jhodgdon’s picture

Status: Needs review » Needs work

Yes, a patch is worth 1000 explanations, to paraphrase. :)

So:
- I think the new context example should be moved up into the first paragraph instead of being part of the second paragraph.
- I think the wording should be "It is also possible to provide a context with the text, similar to t():".
- The rest looks good to me.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.16 KB

Applied those suggestions. I felt the use Drupal\... stuff would have been closer to the simpler example, so people will not stop reading in the middle of the explanation. My idea was that the context stuff is "advanced" info while the use Drupal\... stuff is basic info. But I'm fine with this setup as well.

jhodgdon’s picture

Good point. Maybe we should make it a separate, stand-alone 3rd paragraph then?

Gábor Hojtsy’s picture

This one?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x.

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

Anonymous’s picture

Issue summary: View changes

a possibly stupid 4th idea